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

Improvements to analysis page from the feedback #751

Merged
merged 19 commits into from
Nov 17, 2023

Conversation

danielfdsilva
Copy link
Collaborator

@danielfdsilva danielfdsilva commented Nov 15, 2023

Contributes to US-GHG-Center/veda-config-ghg#221 to get us close to a delivery state.

Includes the work done in #744 plus changes so that custom api endpoints are supported in the exploration page.
Besides the former:

  • Connects the data-catalog to exploration by filtering the dataset selector modal by the veda dataset name
  • Removes connections to previous analysis and dataset explore pages
  • Add info icon to dataset [E & A] Add info icon to dataset card #747
  • Ensure that the analysis range is within the current domain (bug identified in the feedback session)
  • Add support for vector datasets - I noticed that this was no implemented in the A&E so I added it. The zarr layer support is still not implemented, but not sure we need it immediately.

Copy link

netlify bot commented Nov 15, 2023

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit bc5f4af
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/65551005b061dc0008a69d82
😎 Deploy Preview https://deploy-preview-751--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.

@danielfdsilva danielfdsilva marked this pull request as ready for review November 15, 2023 18:32
@danielfdsilva
Copy link
Collaborator Author

danielfdsilva commented Nov 15, 2023

One thing missing: Port modal to view tile endpoints @danielfdsilva

Needed while the code is not deleted to prevent ts errors
@j08lue
Copy link
Contributor

j08lue commented Nov 15, 2023

  • The zarr layer support is still not implemented, but not sure we need it immediately.

If we release this first on VEDA, I guess we will need to have this support, since we have a dataset there that relies on TiTiler-Xarray: NASA-IMPACT/veda-config#323

Copy link
Contributor

@j08lue j08lue left a comment

Choose a reason for hiding this comment

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

The three most visible improvements

Connects the data-catalog to exploration by filtering the dataset selector modal by the veda dataset name
Removes connections to previous analysis and dataset explore pages
Add info icon to dataset #747

Are very nicely done and seem to work well.

Not sure how to reproduce the bug with analysis range out of bounds - I think this was caused by running an analysis on a different dataset/layer, then switching while it was still active...? I trust that the fix is helpful either way and we will see whether this issue occurs again in any subsequent testing.

We should revisit the list of showstoppers for a release in US-GHG-Center/veda-config-ghg#221 (comment) and add any new ones that have up, but this closes some of the main ones.

@danielfdsilva
Copy link
Collaborator Author

@j08lue Alright, I'll port the zarr viz as well. Should be pretty easy since it is only a layer component.

For completion, to replicate the range error on the non fixed version:

  • Add NPP and CO2 datasets
  • Draw AOI and set range to somewhere in 2020 (only co2 has data)
  • Remove CO2 dataset

The bug is that the range will remain in 2020, even though the remaining dataset (NPP) only has data up to 2018. This no longer happens in this PR.

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.

I got some questions/ noticed some items that are not necessarily related to this pr (nothing blocker)

  1. What is the expected behavior for vector, zarr. layer for 'analysis' ? I don't actually know if analysis endpoint works the same for these different layers. Probably @j08lue can answer this question?

  2. I noticed that people are just repeating the same layer (with the same ids) to populate the layers in different datasets ex. mtbs-burn-severity . It seems we rely layer id to component key here and there, ends up warning about the duplicate keys.

  3. Since we introduced 'stacApiEndpoint' for layers, we probably can get rid of XARRRAY_ENDPOINT: https://github.com/NASA-IMPACT/veda-config/blob/develop/.env#L15 from env, and move this info to zarr layer mdx file.

@j08lue
Copy link
Contributor

j08lue commented Nov 16, 2023

Great review comments, @hanbyul-here.

Probably @j08lue can answer this question?

Regarding Analysis on TiTiler-Xarray datasets: no this functionality (/statistics) does currently not exist on TiTiler-Xarray. Can we as a first-off catch the failure when the UI tries to make that call and show an error message to the users?

We need to look into implementing that functionality, but it is also ok that it does not exist - the TiTiler-Xarray dataset is advertised as experimental anyways.

@danielfdsilva
Copy link
Collaborator Author

  1. I think now just shows empty, but I'll check

  2. If they have the same id they'll be considered the same layer, which may not be wrong?! But not sure why you need the same layer on different datasets. Might be good to understand the use case.

@danielfdsilva
Copy link
Collaborator Author

Will merge this and make the remaining tweaks on a separate PR

@danielfdsilva danielfdsilva merged commit b34e3b4 into feature/exploration Nov 17, 2023
7 checks passed
@danielfdsilva danielfdsilva deleted the feature/feedback branch November 17, 2023 11:22
@j08lue j08lue linked an issue Nov 21, 2023 that may be closed by this pull request
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.

[E & A] Add info icon to dataset card
3 participants