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

[E & A] Implement colormap configurability #1117

Merged
merged 45 commits into from
Aug 29, 2024
Merged

Conversation

dzole0311
Copy link
Collaborator

@dzole0311 dzole0311 commented Aug 21, 2024

Related Ticket: #994

Description of Changes

Implements the color map configuration of E&A layers.

Changes:

  • Adds curated list of supported color maps taken from the back-end, split into diverging and sequential groups
  • Supports other valid color map names from the list of color maps: https://openveda.cloud/api/raster/colorMaps
  • Adds support for reversing the colors
  • Adds reset all button which currently only resets the "Reverse" option, but will also apply to the "Rescale" functionality which is to be added next
  • Added a loading skeleton to the colormap in the timeline sidebar when a layer is added. This prevents a visual flash because it might take a second to fetch the color map from the back-end (cc @faustoperez )

The precedence of the color maps are as follows:

  1. Start with what is configured in the sourceParams of the dataset
  2. If it's not defined, check for the dashboard render configuration coming from STAC endpoint
  3. If still undefined, check for asset-specific renders using the sourceParams' assets. This is a special edge case for some datasets that have multiple assets (multiple assets under one collection item)
  4. If all else fails, default to viridis

Please let me know your thoughts on that or if you spot something strange

Notes & Questions About Changes

  • I additionally pushed a shell script that I used to build the colorMaps.ts file, I hope that's okay. It fetches all the rgba values for each supported colorMap from titiler. We could extend it and integrate it within our Github workflow in the future, so that the colors are fetched on each new release (although it's unlikely they will change that often)

Validation / Testing

  • Verify that the color map list is correctly split into diverging and sequential groups
  • Check that reversing the color map works as expected and toggles correctly
  • Test the "Reset All" button to ensure that it resets the "Reverse" option and works as intended
  • Check that the precedence of the color maps is followed in the correct order:
  1. Start with sourceParams of the dataset
  2. Fallback to dashboard render configuration from the STAC endpoint
  3. If undefined, check for asset-specific renders using sourceParams' assets
  4. Finally, default to viridis if all else fails
  • Verify that the changes do not break any existing functionality related to rendering of the tiles on the map (selected color map must match the tile colors)
  • Test across different datasets with both single and multiple assets to confirm edge cases are handled as described

Copy link

netlify bot commented Aug 21, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 60559fd
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/66cf139d23f2670008f3e2cb
😎 Deploy Preview https://deploy-preview-1117--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@faustoperez
Copy link

Added a loading skeleton to the colormap in the timeline sidebar when a layer is added. This prevents a visual flash because it might take a second to fetch the color map from the back-end (cc @faustoperez )

Sounds great @dzole0311 !

Copy link
Collaborator

@hanbyul-here hanbyul-here left a comment

Choose a reason for hiding this comment

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

This needs to be changed if we want to give a priority to manual sourceParams: https://github.com/NASA-IMPACT/veda-ui/pull/1117/files#diff-fc839f8286ca1783e679e0fcddd8a3da452570ebbe9f433e16fda7003b91687eR376 (but hopefully we can move on from manual configurations soon 😬 !)

app/scripts/components/common/map/layer-legend.tsx Outdated Show resolved Hide resolved
@@ -358,7 +374,8 @@ export function RasterTimeseries(props: RasterTimeseriesProps) {
const tileParams = qs.stringify(
{
assets: 'cog_default',
...(sourceParams ?? {})
...(sourceParams ?? {}),
colormap_name: colorMap
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 probably a nitpick, If colormap is optional anyway, can we do

...colorMap &&  {colormap_name: colorMap}

and get rid of the https://github.com/NASA-IMPACT/veda-ui/pull/1117/files#diff-b015ae8d170bf7762b2b8ad8c59673574d21cced66fccabdf7cac68e7d30b864R27?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need this in order to check if the colorMap is part of the url params. Did you mean we could move it one level up or completely get rid of it?

@hanbyul-here
Copy link
Collaborator

One more - do you see any reason not to apply dynamic colormap to other types of raster layers? (cmr and zarr)? - We can separate the issues for these layers since these are not being used on production anyway, but I was wondering if there is any specific reason!

@dzole0311
Copy link
Collaborator Author

Lots of good comments @hanbyul-here, thanks!

One more - do you see any reason not to apply dynamic colormap to other types of raster layers? (cmr and zarr)? - We can separate the issues for these layers since these are not being used on production anyway, but I was wondering if there is any specific reason!

It somehow slipped my mind to pass the colormap to the RasterPaintLayer as well. Should work now for CMR and Zarr layers.

Copy link
Contributor

@aboydnw aboydnw left a comment

Choose a reason for hiding this comment

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

I think that somehow leading users back to the visualization recommended/configured by the science team will be important to stakeholders. Whether that's the reset button also resetting the colormap selection or providing some sort of visual cue which colormap is the default.

I see in the netlify preview that some of the datasets start with "Default," and some didn't. I assume this has to do with the logic you were talking about @dzole0311 to decide which colormap is the default. Is there any way to ensure that whatever is decided to be the default comes with this tag?

Also, another question, do we think users will want to see both Default AND the colormap name? cc @faustoperez

Example w/o default tag:
image

There is also an edge case that I discovered. If it's an easy fix, maybe worth addressing. Otherwise we can make a ticket and do it at some point in the future.
Steps to reproduce:

  1. Open exploration tab and select layers
  2. Change the colormap of one of the layers to something other than the Default
  3. Notice that if you close and reopen the colormap bar, the Default tag is still there
  4. Refresh the page and notice that the change in colormap is maintained (I believe this is desirable, so people can share views with others via URL)
  5. Open the colormap bar and notice that the Default tag is no longer there

return isReversed ? colors.reduceRight((acc, color) => [...acc, color], []) : colors;
};

export function ColormapOptions({ colorMap, setColorMap}: ColormapOptionsProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious, with Jotai state management, cant we access the same state from the same atom? so instead of passing colorMap state and setter method, can we just pass in the datasetAtom and then do const [colorMap, setColorMap] = useTimelineDatasetColormap(datasetAtom); at this level to avoid prop drilling since datasetAtom was already being previously being passed into DataLayerCard?

dzole0311 and others added 5 commits August 28, 2024 13:40
**Related Ticket:** #994 

### Description of Changes
I made the colormap default to be returned along with other settings
values - I think this saves some redundancies of retrieving the value of
colormap, and fits well with the existing pattern.
I eliminated reconciling functions from data-util (and used the function
from data-util-no-faux-module) so there is no confusion about which
function is being called.
Co-authored-by: Hanbyul Jo <hi@hanbyul.xyz>
@dzole0311
Copy link
Collaborator Author

As discussed yesterday, I removed the "Reset All" and "Default" label for now.

I see in the netlify preview that some of the datasets start with "Default," and some didn't. I assume this has to do with the logic you were talking about @dzole0311 to decide which colormap is the default. Is there any way to ensure that whatever is decided to be the default comes with this tag?

@aboydnw This and the other issue you spotted are partially related (ticketed here: #1130) and will be handled together within one fix.

Copy link
Collaborator

@hanbyul-here hanbyul-here 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 amazing work!

@dzole0311 dzole0311 merged commit f0a4ace into main Aug 29, 2024
8 checks passed
@dzole0311 dzole0311 deleted the 994-exploration-colormap branch August 29, 2024 06:30
@faustoperez
Copy link

Also, another question, do we think users will want to see both Default AND the colormap name? cc @faustoperez

"Default" here refers to the colormap set by the science team. When designing this, my understanding was that they often don't name their colormaps or use readily available ones like the Viridis family, hence my use of "default" to mean the original setting.

Perhaps replacing "Default" with "Initial" would make this clearer?

Also, if we can identify whether the science team's colormap is one already listed, we could set it as the default or highlight it in the colormap panel to avoid duplicates 🤷‍♂️

sandrahoang686 added a commit that referenced this pull request Sep 25, 2024
## 🎉 Features
- [E&A] Implement colormap configurability
#1117
- [E&A] Feature flag colormap configurability
#1147
- Add font md size as 20px for new Design System
#1125
## 🚀 Improvements
- [E&A] Return default value of colormap along with other settings
#1128
- Create new raster paint layer module and factor out
BaseTimeseriesProps #1105
- Consolidate a place where to check linkProps
#1121
#1160
- Expose Data Catalog and update child components to pass in routing/nav
for library build #1096
#1159
- Set up eslint rule for no trailing spaces
#1146
- Replace cmr-stac with titiler-cmr
#1131
## 🐛 Fixes
- Update external link in top navigation target
#1145
- Update PageHeader/Nav to not throw
#1149
- [E&A] Fix to convert the time to usertzdate
#1151
- Use sourceparams as it is when it is there
#1148
- Fix compare label on block map
#1153
- Discard the previous sourceExclusive value to fix the case when the
datasets with different sourceExclusive can be selected together
#1161
- Show the name of the selected filter on story hub
#1161
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 this pull request may close these issues.

6 participants