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

Titiler-cmr layer #1001

Closed
wants to merge 20 commits into from
Closed

Titiler-cmr layer #1001

wants to merge 20 commits into from

Conversation

abarciauskas-bgse
Copy link
Contributor

Related Ticket: #1000

Description of Changes

Application code changes - necessary changes

  • renames ZarrPaintLayer to RasterPaintLayer, since it is not specific to Zarr
  • adds a new type titiler-cmr and renames the cmr type to cmr-stac for disambiguation (although it is sure to be a bit confusing, since cmr-stac is for the use case of using the CMR STAC catalog and titiler-cmr is for the use case of tiling data in CMR but those requests are setup via collection records catalogued in VEDA STAC)
  • adds a third hook useTitilerCMR. Because this third hook needs to return tiling parameters besides assetUrl, refactor the other hooks to also return a tileParams dictionary.

Application code - optional changes, simplifying some things

  • Merges/removes duplication across MapLayer*TimeseriesProps to just one interface MapLayerRasterTimeseriesProps since the only thing that seemed to separate these types was assetUrlReplacements. This change is not strictly necessary for this functionality to work but it seemed to simplify things IMO.
  • Simplify conditionals which set the datetime extent and periodicity. The VEDA STAC metadata for these CMR collections uses null as the second datetime value in the temporal extent to indicate ongoing collections. This is the declared way to do so in the STAC collection specification, so it seems valid to me to allow for all non-vector collections to set the end datetime to the current date if it is null in the metadata.

Dataset changes

  • Adds 2 datasets to demonstrate titiler-cmr is working for those datasets. We could go down to one since it seems cumbersome to manage multiple mock datasets for a single use case.
  • Removes the GPM IMERG cmr-stac collection, since this overlaps with the titiler-cmr example and we can already cover the cmr-stac example with the TRMM dataset
  • Adds the cmip6 kerchunk reference dataset, so there is a dataset to test with the "zarr" type

Notes & Questions About Changes

I would be happy to go over these changes with one or more of the core UI developers.

Validation / Testing

I tested these changes by opening up all of the mock datasets explore pages.

Copy link

netlify bot commented Jun 12, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit f793fe5
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/666f7982e1e1150008e204c0
😎 Deploy Preview https://deploy-preview-1001--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.

@abarciauskas-bgse
Copy link
Contributor Author

Will address linting and ts-check later today

@abarciauskas-bgse
Copy link
Contributor Author

Checks are passing but unsure about the changes in style-generators and why it seems like there is duplication of files there. Would love to pair with UI team member(s) to review!

@sandrahoang686
Copy link
Collaborator

@abarciauskas-bgse thanks for detailed description and for the explanation on #1000. I dont mind campfiring on this together for review! It will be more interactive though since I haven't worked with the style-generators much. I'm guessing the duplication of files may be related to this past PR #805 (specifically maybe this commit 0b2dece)

@sandrahoang686
Copy link
Collaborator

sandrahoang686 commented Jun 14, 2024

@abarciauskas-bgse the veda dashboard team is also working on deprecating the /common/mapbox/ directory as we are are moving to consolidate our maps and mapping logic to a the new map provider and logic under dir /common/map/ (see OG ticket here). This is why you may be seeing duplicated logic in both these places. Work is still happening on migrating current maps and logic under the new path. I recommend if possible to try to minimize references to anything under /common/mapbox/ and if the logic doesn't already exist under the new dir /common/map/ if you would be comfortable moving this logic over, I can work with you to do that if as well

@@ -0,0 +1,114 @@
import { useEffect, useMemo } from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔧 : We are working to deprecate this path. The new path for this logic should go under /common/map/. I believe you would want to migrate this over to /common/map/style-generators/.

interface STACforCMRResponseData {
collection_concept_id: string;
renders: Record<string, any>;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder 🤔 if the backend team has json schemas we could generate into typescript interfaces to import, interested.. I'll follow up with them on this

let tileParams = {
concept_id: data.collection_concept_id,
datetime: date,
...sourceParams
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔧 : This matches the naming of local state variable, can we change the name so it isn't confusing.

@@ -38,11 +43,19 @@ export function useZarr({ id, stacCol, stacApiEndpointToUse, date, onStatusChang
controller
});

setAssetUrl(data.assets.zarr.href);
const tileParams = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔧 : This matches the local state variable's name, can we change this so it isn't confusing? Or i'd just set state directly with the object like so...

        if (data.assets.zarr.href) {
          setTileParams({
             url: data.assets.zarr.href,
             time_slice: date,
             ...sourceParams
           });
        }

@abarciauskas-bgse
Copy link
Contributor Author

just a update that I am making some changes based on discussion with @sandrahoang686 (to use the new map component, to fix conflicts and to fix some variable naming). so will move this to draft until that is done and this is ready for review again.

@abarciauskas-bgse abarciauskas-bgse marked this pull request as draft June 14, 2024 21:51
@abarciauskas-bgse abarciauskas-bgse changed the title Ab/titiler cmr Titiler-cmr layer Jun 15, 2024
@hanbyul-here
Copy link
Collaborator

hanbyul-here commented Jun 21, 2024

Can you also provide details on how the temporal extent/summaries should be handled? @abarciauskas-bgse I remember we briefly talked about this before, but it's been a while. We are currently shimming the end date to the current day, and density to DAY if the data is missing the density & temporal extent's end date. Is this still the case? (https://github.com/NASA-IMPACT/veda-ui/blob/main/app/scripts/components/exploration/hooks/use-stac-metadata-datasets.ts)

@abarciauskas-bgse
Copy link
Contributor Author

abarciauskas-bgse commented Jun 25, 2024

@hanbyul-here in a word, yes that is what we are still doing.

In more detail:

  • temporal density: In this line https://github.com/NASA-IMPACT/veda-ui/blob/ab/titiler-cmr/app/scripts/components/exploration/hooks/use-stac-metadata-datasets.ts#L85 we first pick whatever time density is defined in the dataset config, then the stac metadata dashboard:time_density, then fallback to DAY.
  • temporal extent: Here we prefer the temporal extent defined in the collection's summary but we fall back to the collections temporal extent. If the second value of the temporal extent is null we assume this is an ongoing dataset and set the end date to the current datetime here. I can now see one improvement where the most recent datetime which corresponds to the time density interval. Previously we were shimming the end date to the current date only when the dataset's "type" was "cmr", however this logic should apply to all STAC collections since it is valid to use null in temporal extent as a way to represent ongoing datasets.

I noticed the same logic is used in fetchLayerById.

@abarciauskas-bgse
Copy link
Contributor Author

Closing to propose a new design for different layer types

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.

3 participants