Skip to content

Conversation

@colourmeamused
Copy link
Member

@colourmeamused colourmeamused commented Sep 25, 2025

This fixes ODC 1.9 regressions in the DEA_Wetlands_Insight_Tool notebook and should be ready to merge. I cleaned up all the cell output, should I run all the cells in DEA_Wetlands_Insight_Tool.pynb and push with output?

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@GL-S
Copy link
Collaborator

GL-S commented Sep 26, 2025

That's great Abeer! I am about to add just a couple of feedback comments for potential minor adjustments!

@@ -35,7 +35,13 @@
{
Copy link
Collaborator

@GL-S GL-S Sep 26, 2025

Choose a reason for hiding this comment

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

Line #11.    # from datacube.utils.geometry import Geometry, CRS

Maybe worth it to remove the commented line, for a neater look


Reply via ReviewNB

@@ -35,7 +35,13 @@
{
Copy link
Collaborator

@GL-S GL-S Sep 26, 2025

Choose a reason for hiding this comment

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

Line #2.    # gpgon = datacube.utils.geometry.Geometry(poly.geometry[0], crs=poly.crs)

Similarly, it is probably less confusing to a user if the unused line is deleted completely. But I don't know if there is a specific reason to leave the option


Reply via ReviewNB

@@ -35,7 +35,13 @@
{
Copy link
Collaborator

@GL-S GL-S Sep 26, 2025

Choose a reason for hiding this comment

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

Line #3.    # time = ('1985-01-01', '2024-01-01')

Similar to the comments above. Although this was already there in the old version of the notebook.


Reply via ReviewNB

@@ -35,7 +35,13 @@
{
Copy link
Collaborator

@GL-S GL-S Sep 26, 2025

Choose a reason for hiding this comment

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

Converting this cell to markdown rather than raw should fix the visualisation of the table


Reply via ReviewNB

Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is referring to the table after the title "WIT CSV data specification table"

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @GL-S! I will do all these, except the time one that as you said is in the current notebook.

I had to pass the time param explicitly under ODC 1.9 in cell [7] or it runs over 1987-2025 (sloooow).

Copy link
Member

@robbibt robbibt left a comment

Choose a reason for hiding this comment

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

This looks great @colourmeamused! We may revisit and remove the extra time query once this fix is rolled out in datacube (opendatacube/datacube-core#2192), but for now I think this is perfect.

@robbibt
Copy link
Member

robbibt commented Sep 29, 2025

@colourmeamused If you're happy with the notebook, feel free to merge with "Squash and merge"! 🙂

@colourmeamused colourmeamused merged commit 52c4af3 into develop Sep 29, 2025
1 check passed
@colourmeamused colourmeamused deleted the wit-upgrade-odc19 branch September 29, 2025 01:48
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