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

[HeatmapMesh] NaN, +inf, -inf are all displayed the same way #1372

Closed
vallsv opened this issue Mar 1, 2023 · 8 comments · Fixed by #1374
Closed

[HeatmapMesh] NaN, +inf, -inf are all displayed the same way #1372

vallsv opened this issue Mar 1, 2023 · 8 comments · Fixed by #1374

Comments

@vallsv
Copy link

vallsv commented Mar 1, 2023

Describe the bug

+inf, -inf, nan are displayed the same way from HeatmapMesh.

Tested with float16, but it's probably the same with float32.

Expected behaviour

We would expect as default behaviour:

  • NaN -> transparent for
  • +inf -> highest color of the lut
  • -inf -> lowest color of the lut
@vallsv
Copy link
Author

vallsv commented Mar 1, 2023

FYI @t20100

@vallsv
Copy link
Author

vallsv commented Mar 1, 2023

According to @t20100 there is also a typo with the nanColor

        if (isnan(value) || isinf(value) || !isSupported(value)) {
          gl_FragColor = nanColor;
        } else {

const vec4 nanColor = vec4(255, 255, 255, 1);

Which should be clamped 1, 1, 1, 1

Or i guess 1, 1, 1, 0 to be transparent?

@loichuder loichuder changed the title HitmapMesh with NaN, +inf, -inf [HeatmapMesh] NaN, +inf, -inf are all displayed the same way Mar 2, 2023
@loichuder
Copy link
Member

Sounds reasonable to me. I don't have really strong opinions on how these should be displayed.

@axelboc
Copy link
Contributor

axelboc commented Mar 2, 2023

I'm reluctant to change the current behaviour in the viewer, as we haven't had any feedback suggesting to change it. On the contrary, in #1181, the idea is to treat a specific, high value (read from HDF5) as "fill value" by ignoring it from the domain computation and hiding it completely from the visualization (i.e. transparent on heatmap vis; removed on line vis). In a way, we're treating +/-Infinity as the H5Web viewer's default fill value.

Of course, we could add a control in the toolbar to change the colors used for fill values on the heatmap but someone would have to make a case for this feature.

For now, I suggest we make the requested behaviour configurable in HeatmapMesh, perhaps with a prop called fillColor: 'transparent' | 'minmax' | string.

@vallsv
Copy link
Author

vallsv commented Mar 2, 2023

If you want extra options, please take a look at matplotlib API before creating new stuffs https://matplotlib.org/stable/api/_as_gen/matplotlib.colors.Colormap.html bad/over/under and double check with @t20100

@axelboc
Copy link
Contributor

axelboc commented Mar 2, 2023

It was just a suggestion, that's why we're discussing it in an issue. 😊 I'm not familiar with matplotlib's API, so always happy to be pointed to the relevant information, thanks!

@t20100
Copy link
Member

t20100 commented Mar 2, 2023

I'd also expect as default behaviour to have -/+Inf displayed with colors of the extrema of the colormap as any other under/over value (those are value numbers), and NaN (as well as value not supported by the used scale) not being displayed, and then add needed props to configure this for what is needed. For what I'm using, I don't see an immediate need to configure this.

The bad(or nan)/over/under looks the most generic way to do it. I would at least separate the handling of NaN (with e.g. nanColor or badColor) from +/-Inf and over/under values:

  • For the "bad" values, I see interest in choosing between a dedicated color and "transparent" (could even be using discard in the shader) to display what's behind it (possible future use case of multiple tilings displayed one on top of the other at different resolutions but not there yet).
  • For over/under, I'm not sure what configuration is exactly needed:
    • Separate +/-Inf from over/under? Probably not, matplotlib doesn't.
    • Separate over from under or configure both at once? For which need?

@loichuder
Copy link
Member

Separate +/-Inf from over/under? Probably not, matplotlib doesn't.

This means that the current treatment cannot be reproduced since +/-Inf color are currenlty different from the regular over/under values:

Current Future
NaN white transparent
+/- Inf white over/under color
Regular over/under over/under color over/under color

I am not against it. Just stating things clearly.

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 a pull request may close this issue.

4 participants