Skip to content

MATLAB: Add function bfGetPlaneAtZCT.m and improve range checking for bfGetPlane#3444

Merged
dgault merged 4 commits intoome:developfrom
mkitti:develop_matlab_bfGetPlaneAtZCT
Feb 18, 2020
Merged

MATLAB: Add function bfGetPlaneAtZCT.m and improve range checking for bfGetPlane#3444
dgault merged 4 commits intoome:developfrom
mkitti:develop_matlab_bfGetPlaneAtZCT

Conversation

@mkitti
Copy link
Copy Markdown
Member

@mkitti mkitti commented Sep 26, 2019

For MATLAB users, it is confusing on how to use bfGetPlane to get a particular plane at a ZCT coordinate. This pull request adds a function bfGetPlaneZCT that handles that exact scenario and all of the 0-indexing to 1-indexing issues per this thread:
https://forum.image.sc/t/ijm-getdataset-crashing-matlab/27210/9

Additionally this adds a function bfTestInRange.m which provides more efficient range checking than using ismember while producing more readable errors.

bfGetPlane range checking for width and height is done only once.

Mark Kittisopikul added 4 commits September 25, 2019 18:45
@mkitti mkitti changed the title Add MATLAB function bfGetPlaneAtZCT.m and improve range checking for bfGetPlane MATLAB: Add function bfGetPlaneAtZCT.m and improve range checking for bfGetPlane Sep 26, 2019
@mkitti
Copy link
Copy Markdown
Member Author

mkitti commented Sep 30, 2019

It appears I should close this in light of #3183

@sbesson
Copy link
Copy Markdown
Member

sbesson commented Oct 1, 2019

Hi @mkitti, #3183 was opened within the context of the investigation to migrate the Bio-Formats MATLAB toolbox to its own source code repository. The migration is still incomplete and we have not been able to come back and finish it so far. In addition to merging the latest upstream changes, what would be needed is some review on the packaging and daily CI integration, some testing of the generated artifacts as well as a review of the Bio-Formats documentation and release process. Until then, the decoupled repository has been marked as experimental and the production code is still maintained in this repository. I will close #3183 to avoid confusion.

@sbesson sbesson mentioned this pull request Oct 1, 2019
@sbesson
Copy link
Copy Markdown
Member

sbesson commented Oct 1, 2019

On the PR itself, if I understand correctly, the primary benefit of the new range helper method is performance. Do you have a typical number of planes worth using for testing?

About the newly introduced function, I certainly see the benefit of allowing to pass individual z,c,t coordinates rather than linear indices. An alternative implementation would be to keep using the same function name but support both linear indices and triplets in the signature e.g.

bfGetPlane(r, 1);  % Get the first plane
bfGetPlane(r, 1, 1, 1, 1000, 1000); % Get a 1000x1000 tile from the first plane
bfGetPlane(r, [1, 1, 1]); % Get the plane at Z,C,T coordinates (1,1,1)
bfGetPlane(r, [1, 1, 1], 1, 1, 1000, 1000); % Get a 1000x1000 tile from the plane at Z,C,T coordinates (1,1,1)

Do you have particular thoughts about this second option?

@mkitti
Copy link
Copy Markdown
Member Author

mkitti commented Oct 1, 2019

Hi @sbesson,

  1. The new helper method also gives user friendly error messages. It separately detects if the error is because the argument is i) not a scalar, ii) not an integer, or iii) not within the range.
>> test = @(x) bfTestInRange(x,'x',500);
>> test([1 2])
Error using bfTestInRange (line 33)
x value, [1  2], is not scalar

Error in @(x)bfTestInRange(x,'x',500)
 
>> test(1.1)
Error using bfTestInRange (line 40)
x value, 1.1, is not an integer

Error in @(x)bfTestInRange(x,'x',500)
 
>> test(501)
Error using bfTestInRange (line 47)
x value, 501, is not between 1 and 500

Error in @(x)bfTestInRange(x,'x',500)
  1. The new helper method is faster and has much better scaling properties:
    bfTestRange_vs_ismember

  2. Modifying the function signature of bfGetPlane to accept zct coordinates would not be a good idea and could lead to cryptic or silent errors. My intuitive thought would be if vector input were acceptable for the plane index then maybe this would deliver planes i, j, and k.

@mkitti
Copy link
Copy Markdown
Member Author

mkitti commented Oct 3, 2019

In thinking about this, vectorizing the plane index would be a really good idea actually. We would just need to loop lines 73 to the end.

I would be willing to take some responsibility for working on the MATLAB interface and splitting it out. Is there anything I should do to get more connected?

@dgault dgault added this to the 6.3.1 milestone Oct 28, 2019
@dgault
Copy link
Copy Markdown
Member

dgault commented Oct 28, 2019

Hi @mkitti, our apologies for the delay with this MATLAB issue. We will target this PR for the next Bio-Formats patch release 6.3.1.

We our also looking at various options for ways in which yourself and others can contribute to the MATLAB interface development longer term and will come back to you once we have a better process in place.

@mkitti
Copy link
Copy Markdown
Member Author

mkitti commented Oct 29, 2019

Hi @dgault, no worries regarding the delay. Just let me know what would be useful in either the near or long term. For example, I can see that a diff where bfGetPlane.m is not modified might be easier to accept sooner rather than later.

@mkitti
Copy link
Copy Markdown
Member Author

mkitti commented Oct 29, 2019

For the long term, I am wondering if there should be a more consistent interface across the different languages.

Following clij there is something attractive to having an API that works consistently, to the copy and paste level, across Python, Jython, and MATLAB:

https://twitter.com/haesleinhuepf/status/1187367287973728256

@dgault dgault modified the milestones: 6.3.1, 6.4.0 Dec 13, 2019
@dgault
Copy link
Copy Markdown
Member

dgault commented Feb 18, 2020

Having carried out some further testing I am happy to go ahead and include the new functions for the release of 6.4.0. This should offer improved behaviour for users without breaking any existing usage.

Longer term we will likely have to come back and relook at updating the API as part of the larger work on the next gen of OME formats as well as looking into ways that make it easier for the community to contribute.

Thanks again @mkitti for your contributions and my apologies of the long delay in getting this included in an official release.

@dgault dgault merged commit 0544530 into ome:develop Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants