-
Notifications
You must be signed in to change notification settings - Fork 64
Support divergent color ramps #912
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
base: main
Are you sure you want to change the base?
Conversation
|
Integration tests report: appsharing.space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmocean's "balance" is cool :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Nakul! This looks really good so far! I have some thoughts on the implementation details, most importantly around saving the min & max into the project file as symbologyState.min, symbologyState.max and updating singleband pseudocolor (for raster data) to share the same behavior. We can definitely share a common component!
packages/base/src/dialogs/symbology/components/color_ramp/ColorRamp.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/components/color_ramp/ColorRamp.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/components/color_ramp/ColorRamp.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/components/color_ramp/ColorRamp.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/components/color_ramp/ColorRamp.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/components/color_ramp/ColorRamp.tsx
Outdated
Show resolved
Hide resolved
|
Hey @nakul-py I'm a bit overwhelmed with interviewing folks for an internship program. I may be a bit delayed! |
Co-authored-by: Nakul Verma <[email protected]>
3a488a0 to
b478a85
Compare
Co-authored-by: Nakul Verma <[email protected]>
mfisher87
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nakul and I worked on this together for a little bit today. Just leaving some thoughts we talked about -- I'll get to a more comprehensive review on Monday!
packages/base/src/dialogs/symbology/components/color_ramp/ColorRampControls.tsx
Show resolved
Hide resolved
|
cf7fe08 removes the min/max source input fields. But now the rendering is non-deterministic. Try clicking classify, then OK, then re-open symbology, re-classify, and click OK. The rendered data can look different each time. |
This data should come from symbology choices.
|
443d195 resolves the initialization of the OpenLayers Source object's min/max values from the symbology info. This is follow-on work resolving problems introduced in cf7fe08. We still need to initialize the symbology info from the file at initialization time (here, if symbologyState isn't defined, look at the data to find the min and max and save it into symbology state: https://github.com/nakul-py/jupytergis/blob/443d19524930b610859583a03f6d3f2f93d5b68a/packages/base/src/mainview/mainView.tsx#L798-L799) |
|
|
|
Hey @mfisher87
|
| ) => void; | ||
| showModeRow: boolean; | ||
| showRampSelector: boolean; | ||
| renderType: | ||
| | 'Graduated' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | 'Graduated' | |
| 'Graduated' |
Leading | doesn't do anything and can be omitted to avoid confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remove this but it regenerates while formatting unless it give lint error.
packages/base/src/dialogs/symbology/components/color_ramp/ColorRamp.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/components/color_ramp/ColorRamp.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/components/color_ramp/ColorRamp.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/components/color_ramp/ColorRampValueControls.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/tiff_layer/types/SingleBandPseudoColor.tsx
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/components/color_ramp/ColorRampValueControls.tsx
Show resolved
Hide resolved
…g as expected dataMin and dataMax should be attributes of the `source`, not the layer/symbology. This only impacts performance, so let's punt it for later.
These are different things! The data minimum and maximum are attributes of the data, and symbology min/max are user preference.
| let dataMax = image.fileDirectory.STATISTICS_MAXIMUM; | ||
|
|
||
| if (dataMin === undefined || dataMax === undefined) { | ||
| // 2. Try smallest overview if available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // 2. Try smallest overview if available | |
| // 2. Try smallest overview if available | |
| // A consequence of this method is that we will almost never find the **true** minimum and maximum for the whole raster, as overviews are aggregate data, averaging groups of grid cells to calculate lower-resolution grid cells. However, calculating the minimum and maximum from the entire dataset may be prohibitively expensive on CPU and memory! |
There's probably a better way to do this, but we don't need to solve that now. We just need to document the tradeoff we're making :)
Description
Addresses #881
balance,delta,curl,diffandtarnso other colormaps are still show type unknown. Enable users to set aminandmaxvalue and also get self-populatedminmaxvalues for the colormap.Bug Fix - This pr also fixes the selection of bands on SingleBand Pseudocolor
Checklist
Resolves #XXX.Failing lint checks can be resolved with:
pre-commit run --all-filesjlpm run lint📚 Documentation preview: https://jupytergis--912.org.readthedocs.build/en/912/
💡 JupyterLite preview: https://jupytergis--912.org.readthedocs.build/en/912/lite