-
Notifications
You must be signed in to change notification settings - Fork 57
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 :)
> = { | ||
balance: { | ||
name: 'balance', | ||
type: 'Divergent', | ||
colors: [], | ||
criticalValue: 0, | ||
}, | ||
delta: { | ||
name: 'delta', | ||
type: 'Divergent', | ||
colors: [], | ||
criticalValue: 0, | ||
}, | ||
curl: { | ||
name: 'curl', | ||
type: 'Divergent', | ||
colors: [], | ||
criticalValue: 0, | ||
}, | ||
diff: { | ||
name: 'diff', | ||
type: 'Divergent', | ||
colors: [], | ||
criticalValue: 0, | ||
}, | ||
tarn: { | ||
name: 'tarn', | ||
type: 'Divergent', | ||
colors: [], | ||
criticalValue: 0, | ||
}, |
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.
@mfisher87 I think balance and the other four are also divergent?
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.
That is true!
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!
if (COLOR_RAMP_DEFINITIONS[name]) { | ||
COLOR_RAMP_DEFINITIONS[name].colors = colorRamp; | ||
} | ||
|
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.
if (COLOR_RAMP_DEFINITIONS[name]) { | |
COLOR_RAMP_DEFINITIONS[name].colors = colorRamp; | |
} |
Instead of having a new data structure for color ramp information, what do you think about using getColorMapList
to generate all the information needed in its return
value? And updating the COLOR_MAP_NAMES
data structure to include all the information aside from the colors (I think COLOR_RAMP_DEFINITIONS
is a great name for that data structure, so let's combine them under that name?). E.g.:
// `Object.keys()` always returns `string[]`, but sometimes we need a narrower type, ideally without assertion or repetition.
// This should live in `tools.ts` with the similar `objectEntries` function.
const objectKeys = Object.keys as <T extends Record<string, any>>(obj: T) => Array<keyof T>;
// This should probably live in types.ts?
type ColorRampType = "sequential" | "divergent";
interface BaseColorMapDefinition {
type: ColorRampType;
}
interface SequentialColorRampDefinition extends BaseColorMapDefinition {
type: "sequential";
}
interface DivergentColorRampDefinition extends BaseColorMapDefinition {
type: "divergent";
/**
* Where in the colorramp the divergence occurs
* @example 0.5
*/
criticalValue: number;
}
type ColorRampDefinition = SequentialColorRampDefinition | DivergentColorRampDefinition
const COLOR_RAMP_DEFINITIONS = {
viridis: {type: "sequential"},
// ...
balance: {type: "divergent", criticalValue: 0.5}, // 0.5 indicates that the colormap's critical value is in the exact middle
// ...
} as const satisfies {[key: string]: ColorRampDefinition};
const COLOR_RAMP_NAMES = objectKeys(COLOR_RAMP_DEFINITIONS);
type ColorRampName = typeof COLOR_RAMP_NAMES[number];
export interface IColorMap {
name: ColorRampName;
colors: string[];
definition: ColorRampDefinition;
}
// example return value of `getColorMapList`
const colorMaps: Array<IColorMap> = [
{name: "viridis", colors: [/* ... */], definition: {type: "sequential"}},
{name: "balance", colors: [/* ... */], definition: {type: "divergent", criticalValue: 0.5}}
];
import { | ||
COLOR_RAMP_DEFINITIONS, | ||
ColorRampName, | ||
} from '../../../symbology/colorRampUtils'; |
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.
} from '../../../symbology/colorRampUtils'; | |
} from '@src/dialogs/symbology/colorRampUtils'; |
Long chains of ../
can be hard to read :)
) => void; | ||
showModeRow: boolean; | ||
showRampSelector: boolean; | ||
layerType?: 'graduated' | 'categorized'; |
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.
layerType?: 'graduated' | 'categorized'; | |
renderType?: 'graduated' | 'categorized'; |
Let's be careful with terminology, layerType already means "GeoTiffLayer" | "VectorLayer" | "VectorTileLayer" | ...
}; | ||
|
||
const rampDef = COLOR_RAMP_DEFINITIONS[selectedRamp as ColorRampName]; | ||
const rampType = rampDef?.type || 'Unknown'; |
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.
This shows "unknown" most of the time, but the suggestion above to COLOR_RAMP_DEFINITIONS
should fix that. I suggest including the color ramp type in the dropdown instead of outside the dropdown. E.g. in the dropdown it would say "Viridis (sequential)".
|
||
<div className="jp-gis-symbology-row"> | ||
<label htmlFor="critical-value">Critical Value:</label> | ||
<input |
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.
Critical value shouldn't be an input -- instead it should be calculated as the colorRampDefinition.criticalValue
(0.5 with the divergent colormaps we're using) point between min and max so the user knows where the divergence point will lie. Changing this value manually should have no effect on the classification (and currently doesn't).
@@ -76,6 +106,64 @@ const ColorRamp: React.FC<IColorRampProps> = ({ | |||
setSelectedMode={setSelectedMode} | |||
/> | |||
)} | |||
{/* 🔹 Divergent colormap controls */} | |||
{layerType === 'graduated' && rampType === 'Divergent' && ( |
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.
Let's support setting the minimum and maximum in the same way for singleband pseudocolor, too, and regardless of whether the color ramp type is divergent or sequential. We only need to display the critical value for divergent color ramps, though.
Right now singleband psuedocolor already has a setting for min and max, and the choice saves the values into the project file under the sources
key (sources[id][parameters][url][0][min|max]
. But this is really a symbology setting, not an attribute of the source data. We should be saving that information at layers[id][parameters][symbologyState][min|max]
for all render types that support a color ramp except heatmap, i.e. graduated, categorized, singleband. We'll also need to update the geotiff add layer dialog to save this information in the correct place!
We should extract the min/max/critical value fields (though critical value should not be editable) into a separate component so we can share it between the graduated and singleband components! The props for that extracted component can be min
, setMin
, max
, setMax
, and colorRampDefinition
... I don't think we need any others but I could be missing something ;)
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.
When classifying in the radius tab, the min and max fields are shown, but the classify button ignores their values. We should be able to specify the classification range there as well.
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.
For 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.
Yeah, graduated render type in the radius tab!
@@ -104,7 +105,35 @@ export namespace Utils { | |||
selectedRamp: string, | |||
nClasses: number, | |||
reverse = false, | |||
layerType: 'categorized' | 'graduated' = '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.
layerType: 'categorized' | 'graduated' = 'graduated', | |
renderType: 'categorized' | 'graduated' = 'graduated', |
}) => { | ||
const [selectedRamp, setSelectedRamp] = useState(''); | ||
const [selectedMode, setSelectedMode] = useState(''); | ||
const [numberOfShades, setNumberOfShades] = useState(''); | ||
const [criticalValue, setCriticalValue] = useState<number | undefined>(0); | ||
const [minValue, setMinValue] = useState<number | undefined>(-5); |
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.
Let's not set an arbitrary default. We should either leave it blank or use the min/max from the actual data.
I think we have two options:
- Leave them blank and offer a button ("Set to data min & max"? "Fill from data"? "Detect range"? "Use actual range"?) which populates the values from the actual data. This will mean the dialog will load faster and the user will control when the data is read.
- Pre-populate them with the values from the actual data. This will mean the dialog loads more slowly.
The "classify" button should be greyed out unless min & max are populated. Then the classify button will always use the min & max and won't have to calculate them.
What do you think?
{/* Labels below bar */} | ||
<div | ||
style={{ fontSize: '0.75em', color: '#555', fontWeight: 'bold' }} | ||
> | ||
{minValue !== undefined && <div>Min: {minValue.toFixed(2)}</div>} | ||
{maxValue !== undefined && <div>Max: {maxValue.toFixed(2)}</div>} | ||
{isDivergent && criticalValue !== undefined && ( | ||
<div style={{ color: '#555', fontWeight: 'bold' }}> | ||
Critical: {criticalValue.toFixed(2)} | ||
</div> | ||
)} |
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.
The bar communicates all this information already, I don't think we need the additional labels :)
{/* Labels below bar */} | |
<div | |
style={{ fontSize: '0.75em', color: '#555', fontWeight: 'bold' }} | |
> | |
{minValue !== undefined && <div>Min: {minValue.toFixed(2)}</div>} | |
{maxValue !== undefined && <div>Max: {maxValue.toFixed(2)}</div>} | |
{isDivergent && criticalValue !== undefined && ( | |
<div style={{ color: '#555', fontWeight: 'bold' }}> | |
Critical: {criticalValue.toFixed(2)} | |
</div> | |
)} |
Description
Currently, Color Ramp Type only supports Divergent colors such as
balance
,delta
,curl
,diff
andtarn
so other colormaps are still show type unknown. Enable users to set a minimum and maximum value for the colormap.Screencast.From.2025-09-02.17-35-03.mp4
Checklist
Resolves #XXX
.Failing lint checks can be resolved with:
pre-commit run --all-files
jlpm run lint
📚 Documentation preview: https://jupytergis--912.org.readthedocs.build/en/912/
💡 JupyterLite preview: https://jupytergis--912.org.readthedocs.build/en/912/lite