[Do Not Merge] Support divergent color ramps #912
[Do Not Merge] Support divergent color ramps #912nakul-py wants to merge 78 commits intogeojupyter:mainfrom
Conversation
|
Integration tests report: appsharing.space |
There was a problem hiding this comment.
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 <nakulverma.py@gmail.com>
3a488a0 to
b478a85
Compare
Co-authored-by: Nakul Verma <nakulverma.py@gmail.com>
mfisher87
left a comment
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
These are different things! The data minimum and maximum are attributes of the data, and symbology min/max are user preference.
packages/base/src/dialogs/symbology/hooks/useGetSingleBandInfo.ts
Outdated
Show resolved
Hide resolved
|
Looks like we have a bug where changing the band number in the singleband symbology menu doesn't change the rendered output. The identify tool indicates that we should see a difference between the bands! |
| let dataMax = image.fileDirectory.STATISTICS_MAXIMUM; | ||
|
|
||
| if (dataMin === undefined || dataMax === undefined) { | ||
| // 2. Try smallest overview if available |
There was a problem hiding this comment.
| // 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! |
packages/base/src/dialogs/symbology/components/color_ramp/ColorRampSelector.tsx
Show resolved
Hide resolved
| @@ -14,15 +18,28 @@ interface IColorRampProps { | |||
| numberOfShades: string, | |||
| selectedRamp: string, | |||
| setIsLoading: (isLoading: boolean) => void, | |||
| criticalValue?: number, | |||
There was a problem hiding this comment.
Since this is for display only, classifyFunc doesn't need to receive it as an argument anymore. Let's remove it!
| criticalValue?: number, |
| // Typeguard: This should never happen | ||
| return; | ||
| } | ||
| const scaledCritical = |
There was a problem hiding this comment.
| const scaledCritical = | |
| // Get the critical value (divergence point in a divergent colormap) as a data value using equal interval interpolation. | |
| // This is only used if equal interval classification mode is selected. | |
| const criticalDataValue = |
There was a problem hiding this comment.
I removed the code related to critical value because as we removed critical value also from classification function. So not the const criticalDataValue = used nowhere.
packages/base/src/dialogs/symbology/components/color_ramp/ColorRampControls.tsx
Outdated
Show resolved
Hide resolved
| layerParams.symbologyState = { | ||
| ...layerParams.symbologyState, | ||
| dataMin, | ||
| dataMax, |
There was a problem hiding this comment.
dataMin and dataMax aren't being saved to the symbology state (we can see that the JSON isn't being updated). This is because we need to call a function to update the shared model; if we only assign to it, it won't update the model. I don't have that function name at hand and have to jump on to a meeting. Let me know if you have trouble finding it!
Though in my testing, things seem to be working well, so I wonder if this whole effect can be deleted and things will still work?
There was a problem hiding this comment.
Sure we can delete specific useEffet.
|
Nakul and I are on a call together thinking about how we can break this PR in to smaller pieces to make it more reviewable and make it easier to add divergent colormaps in the future. Ideas:
export interface ISequentialColorRampDefinition
extends IBaseColorRampDefinition {
type: 'Sequential';
}
/* Later!
export interface IDivergentColorRampDefinition
extends IBaseColorRampDefinition {
type: 'Divergent';
criticalValue: number;
} */
export interface ICyclicColorRampDefinition extends IBaseColorRampDefinition {
type: 'Cyclic';
}
export type IColorRampDefinition =
| ISequentialColorRampDefinition
| IDivergentColorRampDefinition
| ICyclicColorRampDefinition; |
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.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