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

Supported data types #85

Closed
martinschorb opened this issue Mar 2, 2022 · 25 comments
Closed

Supported data types #85

martinschorb opened this issue Mar 2, 2022 · 25 comments

Comments

@martinschorb
Copy link

We should aim making the supported data types consistent across MoBIE tools and NGFF.

originated from: mobie/mobie-utils-python#42 (comment)

@constantinpape
Copy link
Collaborator

To be more precise, this is a data range and not a data type issue; the problem is mainly that the mobie viewer does not handle negative values well because of underlying functionality in BDV.
(As such this is likely not a problem relevant in mobie-io, but rather in mobie-viewer-fiji, where most of the viewer related logic is implemented. But I will wait for @tischi to comment here who would know this better.)

@martinschorb
Copy link
Author

If we provide the range in the view definition (which we do), couldn't a potential (even negative) offset be applied to simply always have 0 as starting value?

@martinschorb
Copy link
Author

I just tried converting the data to N5 using the Fiji-Plugin and confirm that the data type stays float32 and BDV cannot handle it because the intensity range is limited to being positive and integers. (data range in my normalized reconstruction is roughly: -0.002 to +0.002)

@tischi
Copy link
Collaborator

tischi commented Mar 2, 2022

@martinschorb do you have such an image for me that I could try to open?

@martinschorb
Copy link
Author

/g/emcf/Hamburg_XRay/new_rec/PROCESSED_DATA/TAT_06_01_phr_0p2_fullRec/slice_1234.tif

@tischi
Copy link
Collaborator

tischi commented Mar 2, 2022

I mean one that has been converted N5 or OME-Zarr.

@tischi
Copy link
Collaborator

tischi commented Mar 2, 2022

In fact, to really test this it would have to be an image within a MoBIE project. Do you have this?

@martinschorb
Copy link
Author

/g/emcf/Hamburg_XRay/new_rec/PROCESSED_DATA/test_float32.n5
/g/emcf/Hamburg_XRay/new_rec/PROCESSED_DATA/test_float32.ome.zarr

@martinschorb
Copy link
Author

not yet, because I got stuck during Constantin's validation steps (re data range).

@tischi
Copy link
Collaborator

tischi commented Mar 2, 2022

OK, I guess could still try with the /g/emcf/Hamburg_XRay/new_rec/PROCESSED_DATA/test_float32.ome.zarr. I'll let you know!

@martinschorb
Copy link
Author

These are reconstructed X-ray tomograms from HH, so we expect loads of this type of data coming in in the future.

@tischi
Copy link
Collaborator

tischi commented Mar 2, 2022

One can open it in MoBIE, but clicking the [B] button throws:

Exception in thread "AWT-EventQueue-0" java.lang.IllegalArgumentException: (minimum <= value <= maximum) is false
	at javax.swing.SpinnerNumberModel.<init>(SpinnerNumberModel.java:125)
	at javax.swing.SpinnerNumberModel.<init>(SpinnerNumberModel.java:164)
	at bdv.tools.brightness.SliderPanelDouble.<init>(SliderPanelDouble.java:100)
	at org.embl.mobie.viewer.ui.UserInterfaceHelper.showBrightnessDialog(UserInterfaceHelper.java:124)
	at org.embl.mobie.viewer.ui.UserInterfaceHelper.lambda$createImageDisplayBrightnessButton$14(UserInterfaceHelper.java:759)

I will investigate more later today.

@tischi
Copy link
Collaborator

tischi commented Mar 2, 2022

@martinschorb

I can make it work within MoBIE:

image

The question is how to set the maximal range of the contrast limit sliders.

I am using this code right now:

		final double currentContrastLimitsMin = converterSetups.get( 0 ).getDisplayRangeMin();
		final double currentContrastLimitsMax = converterSetups.get( 0 ).getDisplayRangeMax();
		final double absCurrentRange = Math.abs( currentContrastLimitsMax - currentContrastLimitsMin );

		double rangeMin = currentContrastLimitsMin - 3 * absCurrentRange;
		double rangeMax = currentContrastLimitsMax + 3 * absCurrentRange;

Does that make sense? The last two lines I mean? Is a factor of 3 a good idea?

If people want to change beyond rangeMin or rangeMax they have to close and reopen the UI, then it will compute new rangeMin and rangeMax based on the new current values.

Hmm, I should probably also limit by the data type (on top of this).
The issue is that for float the actual Float.MIN and Float.MAX values do not make any sense. For UNIT8 that's a different story.

@tischi
Copy link
Collaborator

tischi commented Mar 2, 2022

@constantinpape @K-Meech would be also good to have your opinion on the above question.

@martinschorb
Copy link
Author

is this getDisplayRangeMin() and Max taken in the center of the volume?

I guess there you should capture most of the intensity range (for this type of datasets). Then, a factor of 3 is already pretty generous (I would maybe go for 1 instead).
However in general cases, if there are some strange things happening at the beginning/end of the data that is not shown in the initial view/slice (and could be interesting in some use case), it still could be outside that range even with 3 or more.
Think of an Xray of a tooth that shows reasonable gray values everywhere organic but has that one small region somewhere where there's the metal inlay with completely crazy intensity values but maybe still interesting.

The OME-Zarr metadata does not (yet?) contain a global min/max that we could read, correct?

@tischi
Copy link
Collaborator

tischi commented Mar 2, 2022

is this getDisplayRangeMin() and Max taken in the center of the volume?

Initially this is taken from dataset.json, and this is populated from the current contrast limits in Fiji when saving the data (when using the ProjectCreator). I do not know how @constantinpape fills those values in python.

Then, a factor of 3 is already pretty generous (I would maybe go for 1 instead).

OK, let's try factor 1

The OME-Zarr metadata does not (yet?) contain a global min/max that we could read, correct?

Not yet, and there is a discussion in one of the issues, and I think the consensus was that that iot should only be an optional field, because for a distributed PB sized data set it may be hard to get those values 😄

@tischi
Copy link
Collaborator

tischi commented Mar 2, 2022

@martinschorb

Here you can try to help that your wishes come true (whatever they are).
ome/ngff#78

@tischi
Copy link
Collaborator

tischi commented Mar 2, 2022

I made a PR to better support B&C settings of float images. We can merge once everyone's happy.

@constantinpape
Copy link
Collaborator

The question is how to set the maximal range of the contrast limit sliders.

I am a bit confused by this question and the ensuing discussions. These values should be read from contrastLimits in the MoBIE metadata.

I do not know how @constantinpape fills those values in python.

By default I am using [0, dtype.max] for integer types and [0, 1] for float types. But users can easily over-write these values.
(And I don't want to include any more heuristics here on the python side; if a user wants something custom they need to pass these values.)

I think that the crucial question is whether we can allow negative values for the contrast limits. I don't mind this at all; it's no issue on the python side and simple to change in the spec. And as far as I understand the discussion here it's also possible in MoBIE-Fiji now.

@martinschorb
Copy link
Author

It makes total sense to take the contrast limits from the respective view entry. It just wasn't immediately obvious from the methods' nomenclature where it gets them from. DisplayRange vs. sourceDisplays -> imageDisplay -> contrastLimits.

So the question would be more on the creator's side (when creating the view) to fetch these values from existing metadata or, when doing the conversion to a MoBIE data format, to provide some means of extracting them during the conversion.
Alternatively, we can decide to offload the contrast limit determination entirely to the user. Then the project creator would need some input to provide these values as you can already do in Python (allowing negative values for float data). Still the user needs to find these limits from somewhere.

The conceptual issue I have here:
As of now, the view contrast limits supersede everything already specified in the (optional as in OME-Zarr) raw metadata or elsewhere. The problem with this is, that in order to populate these (absolute) values properly, the data type needs to be known. An imageDisplay contains a single set of contrastLimits expects a list of sources without specifying their data type (expecting it to be consistent). So there is potential conflict when providing the limits as absolute values and having sources of different data types.
Does that ever happen in real life? What would be an application where multiple sources are set to the same contrastLimits using a single imageDisplay?
It is obvious that these different sources would then require to be put into separate imageDisplays within a set of sourceDisplays for that view.

@tischi
Copy link
Collaborator

tischi commented Mar 3, 2022

@constantinpape

These are actually three different things (see also ome/ngff#78).

  1. contrastLimits, those are used to render the image, they are initially taken from dataset.json but the iuser can change those
  2. contrastUIRange, this is just a rendering issue for the GUI to set the contrastLimits (in fact napari also suffers from this and makes it sometimes impossible to set the contrastLimits https://forum.image.sc/t/contrast-of-inside-napari-cannot-be-adjusted-properly/55330): if there are graphical sliders they cannot range from +/- inf but need to be limited.
  3. valueRange, the actual min and max values of the data (if known, they may be used for contrastUIRange).

@constantinpape
Copy link
Collaborator

Short summary of meeting with @martinschorb and @tischi :

  • contrastLimits: we remove the constraints of the range in [0, uint16.max] from the spec
  • contrastUIRange: we add contrastLimit.max - contrastLimit.min on each side of the slider
  • valueRange: not relevant for now

I will make the spec change and ping you @martinschorb once it's ready to use

@constantinpape
Copy link
Collaborator

@martinschorb you can set arbitrary contrast limits now, but you first need to clear the local copy of the spec:

rm ~/.mobie/*.json

Let me know if you run into any more issues with the project creation.

@tischi
Copy link
Collaborator

tischi commented Mar 3, 2022

I merged the mobie-viewer-fiji PR.
Can we close the issue?

@constantinpape
Copy link
Collaborator

I merged the mobie-viewer-fiji PR.
Can we close the issue?

Yes, sounds good. @martinschorb if there are any more issues with the python creator just raise them in mobie-utils-python

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

No branches or pull requests

3 participants