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

Hv addprofilemapping #22

Merged
merged 15 commits into from
Sep 27, 2024
Merged

Hv addprofilemapping #22

merged 15 commits into from
Sep 27, 2024

Conversation

HansVRP
Copy link
Contributor

@HansVRP HansVRP commented Jul 26, 2024

No description provided.

@jdries
Copy link
Contributor

jdries commented Sep 19, 2024

@HansVRP I ran the notebook myself, on my max ndvi example:

  1. I made small improvements based on that experience
  2. The heatmap has an issue, spatial and temporal range seem to be expected in the dataframe, but weren't there in my case
  3. I would avoid adding NDWI udp
  4. We'll want to integrate the code perhaps with python library

Otherwise, this is a great tool, and would be good to get it merged so we can include it in documentation and SRR.

@HansVRP
Copy link
Contributor Author

HansVRP commented Sep 20, 2024

@jdries Hi Jeroen this was really a dummy example. Let's discuss the requisites for the underlying UDPs and then I can properly expand the benchmark/visualisation to cover all requirements

@jdries
Copy link
Contributor

jdries commented Sep 20, 2024

Hi @HansVRP , the goal for SRR in octobre is to have an MVP that is sufficiently visible to the reviewers. So in terms of functionality, this is already enough, we just need to increase visibility by putting it in apex docs and out of a pr.
It is fine to clearly label it as MVP/prototype, to avoid bad experiences.

If fixing the heatmap seems to complex, we can also just not use it, or show a screenshot.

@HansVRP
Copy link
Contributor Author

HansVRP commented Sep 20, 2024

I'll clean up the notebook and fix the heatmap

@HansVRP
Copy link
Contributor Author

HansVRP commented Sep 23, 2024

@jdries should be good for review. Stepped away from the class based implementation and cleaned up the notebook to make it more explanatory

@jdries
Copy link
Contributor

jdries commented Sep 27, 2024

ok:
ndwi.json is probably not necessary anymore?

There's this issue:
#15

So not sure what to do, just merge this one and then in follow up work merge both projects and also move this code to a better location?

@HansVRP
Copy link
Contributor Author

HansVRP commented Sep 27, 2024

that sounds indeed like a good follow-up step

@HansVRP HansVRP merged commit 797ee3b into main Sep 27, 2024
1 check passed
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.

2 participants