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

Exactextract Tutorial #241

Merged

Conversation

tm-jc-nacpil
Copy link
Collaborator

@tm-jc-nacpil tm-jc-nacpil commented Jul 17, 2024

Adds tutorial for exactextract zonal stats as a new section at the end of the current notebook

So far, I've written the ff.

  1. Replicating the same tutorial (adm1, grid AOIs, bing tile AOIs), just with exactextract
  2. A simple multiband example

Things I think we could add -- lmk your thoughts on this!

  1. Speed test between basic and exactextract functions
  2. Weighted statistics (technically supported by exactextract, but I haven't actually tried this yet, see: https://github.com/isciences/exactextract?tab=readme-ov-file#supported-statistics)
  3. include_cols and include_geoms params (I think not super needed since it's just changing the output table but not what's calculated)

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@tm-danna-ang
Copy link
Collaborator

Things I think we could add -- lmk your thoughts on this!

Hi @tm-jc-nacpil! Lgtm 😄 just wondering why the output wasn't save for Bing Tile Grid Tile AOIs (exactextract).

  1. Speed test between basic and exactextract functions

Sounds good! Maybe we can have a separate notebook for the benchmarking.

  1. Weighted statistics (technically supported by exactextract, but I haven't actually tried this yet, see: https://github.com/isciences/exactextract?tab=readme-ov-file#supported-statistics)

Not sure if other Geo are using weighted statistics so we can put this on low prio for now unless if you already have a use case for it.

  1. include_cols and include_geoms params (I think not super needed since it's just changing the output table but not what's calculated)

You can add a brief note in the tutorial notebook on how to use them or describe sample use cases.

@@ -114,7 +114,7 @@
},
Copy link
Collaborator

@butchtm butchtm Jul 18, 2024

Choose a reason for hiding this comment

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

shouldn't this be a call to rzs.create_exactextract_zonal_stats ?


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for the catch! fixed in the latest commit :D

@tm-jc-nacpil tm-jc-nacpil marked this pull request as ready for review July 29, 2024 08:09
@tm-jc-nacpil tm-jc-nacpil changed the title [WIP] Exactextract Tutorial Exactextract Tutorial Jul 29, 2024
@tm-jc-nacpil
Copy link
Collaborator Author

Latest commit adds the ff.!

  • Speed and output result comparisons between exactextract and rasterstats
  • Addition of recommendations when to use the rasterstats and exactextract modules
  • Examples for using include_cols and include_geom
  • Minor bug fix for handling single dict aggregations

notebooks/03_raster_zonal_stats.ipynb Show resolved Hide resolved
@@ -354,8 +354,8 @@
"name": "stdout",
Copy link
Collaborator

@tm-danna-ang tm-danna-ang Aug 1, 2024

Choose a reason for hiding this comment

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

Line #2.    # Single band

This cell and the next one seem to be both effectively single band with multiple aggregations. You could

  1. Remove this cell
  2. Call this as Single band with single aggregation and then only use one func

Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for the catch, makes sense! let's go with number 2 :D

notebooks/03_raster_zonal_stats.ipynb Show resolved Hide resolved
@@ -114,7 +114,7 @@
},
Copy link
Collaborator

@tm-danna-ang tm-danna-ang Aug 1, 2024

Choose a reason for hiding this comment

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

You could change the heading to Comparison of exactextract and rasterstats in general since you talk about the recommendations and percent difference later on.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in latest commit!

notebooks/tutorial.raster_zonal_stats.ipynb Show resolved Hide resolved
@tm-danna-ang
Copy link
Collaborator

The tutorial looks great! 😄 Just have some alternate suggestions for a few parts.

Copy link
Collaborator

@butchtm butchtm left a comment

Choose a reason for hiding this comment

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

lgtm!

@butchtm butchtm merged commit 6254145 into thinkingmachines:master Aug 7, 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.

3 participants