Skip to content

Add cmocean colormaps, fix bug when switching render type #886

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Aug 27, 2025
Merged

Add cmocean colormaps, fix bug when switching render type #886

merged 13 commits into from
Aug 27, 2025

Conversation

nakul-py
Copy link
Contributor

@nakul-py nakul-py commented Aug 21, 2025 •
edited
Loading

Description

  • Adding cmocean colormaps to the project

CmoceanCompressed.mp4

  • FIxing this small ColorRamp bug

This bug is about that when we click on any color on the Color Ramp the selected color will not be seen in the Color Ramp and Now it is fixed and we can easily see the color that we have selected on the Color Ramp.

Cmocean-bug.mp4

  • Also fixed this symbology change bug

The bug that when we change our symbology Graduated ➜ Heatmap is easily done but when try to change our symbology to Heatmap ➜ Graduated or Categorized, we can't able to do that as you can see in the video.

Before

Bug-Before.mp4

Now we can see in this video Graduated ➜ Heatmap and then Heatmap ➜ Graduated and the same with Categorized.

After

After-Bug-Solved.mp4

Checklist

  • PR has a descriptive title and content.
  • PR description contains references to any issues the PR resolves, e.g. Resolves #XXX.
  • PR has one of the labels: documentation, bug, enhancement, feature, maintenance
  • Checks are passing.
    Failing lint checks can be resolved with:
    • pre-commit run --all-files
    • jlpm run lint

📚 Documentation preview: https://jupytergis--886.org.readthedocs.build/en/886/
💡 JupyterLite preview: https://jupytergis--886.org.readthedocs.build/en/886/lite

@github-actions GitHub Actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch nakul-py/jupytergis/cmocean

@mfisher87 mfisher87 added the enhancement New feature or request label Aug 21, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 21, 2025 •
edited
Loading

Integration tests report: appsharing.space

mfisher87
mfisher87 reviewed Aug 21, 2025
View reviewed changes
Copy link
Member

@mfisher87 mfisher87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this work, @nakul-py ! I didn't have time today to get all the way through, so I'll submit this review now and finish tomorrow :) 🙇

packages/base/src/dialogs/symbology/components/color_stops/StopRow.tsx Outdated
id={`jp-gis-color-color-${index}`}
ref={inputRef}
value={rgbArrToHex(outputValue)}
value={rgbArrToHex(outputValue as number[])}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to use a typeguard instead of a cast? Perhaps by extracting this conditional to a function or immediately invoked function expression?

@nakul-py nakul-py requested a review from mfisher87 August 22, 2025 06:16
@nakul-py nakul-py requested a review from mfisher87 August 22, 2025 16:10
mfisher87
mfisher87 requested changes Aug 22, 2025
View reviewed changes
Copy link
Member

@mfisher87 mfisher87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies in advance for the larger request at the end 😬 This is excellent work, and just needs a little bit of simplification!

packages/base/src/dialogs/symbology/components/color_stops/StopRow.tsx Outdated
| [number, number, number]
| [number, number, number, number];
type HexColorValue = string;
type InternalRgbArray = number[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very generous of you to implement this naming improvement, thank you! What does the InternalRgbArray represent here? What might be an example value that doesn't fit with the RgbColorValue type? If it's complicated to implement the type as string | [number, number, number] | [number, number, number, number] and we need to bring in number[], perhaps we should make type RgbColorValue = number[] and come back to it later :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added InternalRgbArray as a fallback if we don’t want strict enforcement of must be 3 or 4 elements.

@nakul-py nakul-py requested a review from mfisher87 August 24, 2025 07:51
@nakul-py nakul-py changed the title Adding cmocean colormaps Adding cmocean colormaps and Fixing Bugs Aug 24, 2025
@mfisher87
Copy link
Member

Hey @nakul-py I'd like to be more specific in the issue title about which bug is being fixed. I wasn't sure from the video. Could you provide some detail on that?

@nakul-py
Copy link
Contributor Author

Hey @nakul-py I'd like to be more specific in the issue title about which bug is being fixed. I wasn't sure from the video. Could you provide some detail on that?

@mfisher87 now i have updated the pr description that helps you to understand the small bug. ;)

@mfisher87 mfisher87 changed the title Adding cmocean colormaps and Fixing Bugs Add cmocean colormaps, fix bug when switching render type Aug 25, 2025
@mfisher87
Copy link
Member

Thanks @nakul-py ❤️ I updated the description. In the future, multiple smaller PRs doing only 1 thing will be easier to review and faster to merge!

mfisher87
mfisher87 requested changes Aug 25, 2025
View reviewed changes
Copy link
Member

@mfisher87 mfisher87 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for keeping up with all my requests, @nakul-py . A few more here to simplify things down. My intent here is that we have only two changes in this PR to merge it:

  • The bugfix for the blank dialog when changing "render type" dropdown
  • Adding the new colormaps to the colormap library's data structure colorScales. Because we use colormap() as our interface to the colormaps already, this should enable us to keep all of the internals the same -- no special cases for cmocean.

packages/base/src/dialogs/symbology/vector_layer/types/Heatmap.tsx Outdated
Comment on lines 65 to 83

const colorMap = colormap({
colormap: selectedRampRef.current,
nshades: 9,
format: 'hex',
});
const cmoRamp = (cmocean as any)[selectedRampRef.current];
let colorMap: string[];

if (cmoRamp) {
const fullRamp = colormap({
colormap: selectedRampRef.current,
nshades: cmoRamp.length,
format: 'hex',
});
const step = Math.floor(fullRamp.length / 9);
colorMap = Array.from({ length: 9 }, (_, i) => fullRamp[i * step]);
} else {
colorMap = colormap({
colormap: selectedRampRef.current,
nshades: 9,
format: 'hex',
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this special case for cmocean colormaps? My goal with the change to updating the colormap colorScale is so we shouldn't have any special cases.

mfisher87
mfisher87 reviewed Aug 25, 2025
View reviewed changes
mfisher87
mfisher87 requested changes Aug 26, 2025
View reviewed changes
Copy link
Member

@mfisher87 mfisher87 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking amazing. Thanks for the extra effort to increase code quality in the areas we're touching!!

The biggest thing still on my mind is how to handle the need for resampling the colormaps when the colormap() library can't do it -- it can only upsample, but not downsample, colormaps. So we upsample every colormap to 256 shades, then based on the number of shades the user requests, we select N number of evenly-spaced shades from those 256. The downside of this approach is we've discretized the colormap to 256 shades, and we no longer select exactly equally spaced shades (for example, 256 doesn't divide evenly by 11).

I think the best solution to this is to downsample the colormaps in cmocean.json to 8 shades where possible. oxy is a special case where it needs stops at 20% and 80%; I think we should just remove oxy for now to make this simpler. Openlayers is also linearly interpolating oxy between the user-selected classes, so we need to solve for that as well.

I see one remaining bug: When I input 1 for "Classes", the dialog crashes. -1 and 0 produce no classification results without crashing the dialog, which I think is fine. Let's try and get 1 to behave the same as 0.

@nakul-py nakul-py requested a review from mfisher87 August 27, 2025 06:55
mfisher87
mfisher87 requested changes Aug 27, 2025
View reviewed changes
Copy link
Member

@mfisher87 mfisher87 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the last thing I think we need to address before merge. Awesome work!

I hope you don't mind -- since there's so little left I'm just going to apply the suggestions in the GitHub interface and merge once checks pass :)

@mfisher87
Change colormap/colorScale.js stub to empty
534bb62 
Even without content in this module declaration, typescript successfully infers a well-structured type when importing from this module.
@mfisher87
Let the compiler infer type from cmocean.json import
7e06d2c
mfisher87
mfisher87 approved these changes Aug 27, 2025
View reviewed changes
Copy link
Member

@mfisher87 mfisher87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any last minute thoughts @martinRenou @arjxn-py @gjmooney ?

@mfisher87 mfisher87 merged commit ae02cb7 into geojupyter:main Aug 27, 2025
14 checks passed
mfisher87 added a commit to mapninja/jupytergis-docwork that referenced this pull request Aug 28, 2025
@nakul-py @mfisher87
Add cmocean colormaps, bugfixes (geojupyter#886)
e911b85 
Co-authored-by: Matt Fisher <[email protected]>
@nakul-py nakul-py deleted the cmocean branch August 28, 2025 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfisher87 mfisher87 mfisher87 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support cmocean colormaps
2 participants
@nakul-py @mfisher87