trackmate-sc / MaMuT

The fiji plugin for Massive Annotation
http://imagej.net/MaMuT
GNU General Public License v3.0
6 stars 6 forks source link

Update TGMMImporter2.java #24

Open mhdominguez opened 3 years ago

mhdominguez commented 3 years ago

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 commented 3 years ago

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.

mhdominguez commented 3 years ago

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 commented 3 years ago

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

ksugar commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.