BIOP / bigdataviewer-biop-tools

Another repo for bdv tools.
5 stars 2 forks source link

Export ImagePlus with more than one time point crashes #30

Closed tischi closed 3 years ago

tischi commented 3 years ago

@NicoKiaru

The exporter seems to have an issue with images that have more than one timepoint.

image

Do you have time to fix this or should I give it a go?

NicoKiaru commented 3 years ago

Hum, ok, maybe one of these 0 vs 1 index base story.

tischi commented 3 years ago

Will you have a look or shall I?

tischi commented 3 years ago

@NicoKiaru Hm, we have some not reproducible situation. I have the feeling that this method:

SourceAndConverterHelper public static int getMaxTimepoint(SourceAndConverter<?> sac)

....sometimes does not return the correct number of time points even though we can see all of them in the BDV window. Do you have any suspicion?

tischi commented 3 years ago

Aha, maybe it is the following:

public static int getMaxTimepoint(SourceAndConverter<?> sac)
...
        if (!sac.getSpimSource().isPresent(0)) {
            return 0;
        }

Maybe some sources are initialized with 1 being the first time point and not 0?

tischi commented 3 years ago

I found something weird:

image

Maybe your code fails for sources where the number of time points is a multiple of 2?

tischi commented 3 years ago

@NicoKiaru (ping in case you did not get notified anyway)

tischi commented 3 years ago

I think this can simply be fixed by adding a +1 to the for loop.

for (int tp = previous;tp<iFrame+1;tp++)

If you agree, I would be grateful if you could just make the change such that we don't have to go through the whole PR stuff 😃

While we do this, I think we should also rename nFrames to maxFrame to avoid confusion.

0,1,2,3,4: nFrames = 5; maxFrame =4

tischi commented 3 years ago

I realize now that in fact you are returning numFrames, which means that maybe the name of the method is wrong? getMaxTimepoint should be getNumTimepoints?!

tischi commented 3 years ago

@NicoKiaru Just add 1:

was: for (int tp=previous;tp<iFrame;tp++)

should be: for (int tp=previous;tp<iFrame+1;tp++)

NicoKiaru commented 3 years ago

I do not observe any issue with the command and dataset with timepoints:

image

In the command, the timepoint is 0 based. Maybe that's where your issue comes from ?

tischi commented 3 years ago
  1. One issue is that if you have a data set with 2^N time points it will only export 1. I pasted a screenshot of the bug in the code above.
  2. The second issue is, I think, that the actual numbers that one should enter are 0 based, but the example text in the GUI is 1 based.
tischi commented 3 years ago

You can test by having a data set with 16 time points.

NicoKiaru commented 3 years ago

Thanks! Good catch!

tischi commented 3 years ago

Thanks for the fix! I think we should also change the examples in the Command UI to be zero based! This would be much less confusing for the users.

NicoKiaru commented 3 years ago

I changed the comments in the UI in the 'basic export current view blabla'. Did I miss other places?

tischi commented 3 years ago

I think that was the main place. Thanks ❤️ BTW: I am on holidays now for a week 🌴 😄

tischi commented 2 years ago

@NicoKiaru Are those fixes already on the update site?

NicoKiaru commented 2 years ago

No the bigdataviewer-playground update site is old. You can use ABBA for now. But there's a paper coming so I need to update the bigdataviewer-playground update site this week.

tischi commented 2 years ago

@NicoKiaru On ABBA update site the example is still wrong:

image

The examples should be zero based.

NicoKiaru commented 2 years ago

oki doki