Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update TGMMImporter2.java #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mhdominguez
Copy link

TGMMImporter2::process() has at least four places where loop index integer t and integer frames[t] are confused, see proposed commit request. The bug results in MaMuT not being able to import TGMM results series that begin on frames other than 0 -- even if user sets tFrom / tTo correctly. Following these changes, importer works correctly with and without "crop on import." Notably, Mastodon does not have this bug, and is able to import TGMM results starting and ending at any arbitrary time frame number.

@tinevez
Copy link
Member

tinevez commented May 6, 2021

Thanks!
I will look into it. Some of the changes that are in were proposed by Ko if I remember and I need to check how we could have this now.

@tinevez tinevez self-requested a review May 6, 2021 08:00
@tinevez tinevez self-assigned this May 6, 2021
@tinevez tinevez added the bug label May 6, 2021
@mhdominguez
Copy link
Author

I just realized this proposal is the exact reversal of Pull Request #17, merged on 15-Feb-2018! Let me look into this some more, so I can investigate the behavior that was fixed with that PR and try to reconcile. If out-of-order XML listing caused the problem, it may be more easily fixed by adding something like "Arrays.sort(xmlFiles);" to line 173....though I have to look into it.

The problem I am seeing with the current code...is that, let's say you want to import TGMM results for timeframes 30-49, and you only have XML files for those timepoints...frames.length will equal 20, but the first frame is 30, so line 197 will try to fill the thirtieth item in array frames with a zero, even though the total array is only 20 items long...and line 204 will catch the oob exception.

@tinevez
Copy link
Member

tinevez commented May 7, 2021

Ok, I'll wait. Maybe we could bring @ksugar to the discussion? As he authored #17.

@ksugar
Copy link
Contributor

ksugar commented May 8, 2021

When I wrote the pull request #17, I had the issue #20.
It would work if tFrom == 0. When 0 < tFrom, it would cause an IndexOutOfBoundsException as you mentioned.
As far as I remember, the problem was the order of xmlFiles.
Applying your changes and adding Arrays.sort(xmlFiles); would solve the problems.
Thank you for pointing out this issue.

@mhdominguez
Copy link
Author

Thanks ksugar. I have now replicated the bug that was fixed with PR #17; oddly it doesn't occur every time, even for the same dataset, so clearly file.listFiles is not consistent in how it reports the directory listing. I'm still doing some testing on my end; the Arrays.sort(xmlFiles); solution feels like a hack since it uses the file order to pass the correct temporal sequence to the importer methods, but it might be the most efficient in terms of lines of code. Will reply with some updates when I have them.

@mhdominguez
Copy link
Author

mhdominguez commented Jun 1, 2021

Sorry this is taking awhile. After real-world testing, I updated the pull request to include additional changes that were necessary to allow the TGMM importer to handle middle time points (not starting at zero), as well as to import TGMM with BDV datasets that start on non-zero time points. So far, the squashed commit 0684d10 appear to work well, but I am doing a little more testing and will be back in touch when I feel confident that everything is in good shape.

…ther than 0 -- even if user sets `tFrom` / `tTo` correctly. Following these changes, importer works correctly. Additional changes were made to ImportTGMMAnnotationPlugin_.java to pass list of timepoints for which affine registration models are available, so those can be properly matched with the incoming TGMM data.

	modified:   src/main/java/fiji/plugin/mamut/ImportTGMMAnnotationPlugin_.java
	modified:   src/main/java/fiji/plugin/mamut/io/TGMMImporter2.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants