Open kls2177 opened 1 year ago
Looks like this points to a bug in xagg as well. I can start an issue there and hopefully take a stab at it. It's high time for a new release anyways.
couldn't get the conda install of xagg to work. Used pip but had to also add xesmf. got a "warning" for the weightmap = xa.pixel.overlaps() step (see attached screenshot) got an error for the xa.aggregate step (see attached screenshot)
This is now fixed in xagg
v0.3.1, which is the latest version on conda-forge. Currently conda
and mamba
still want to download v0.3.0.2 (which has a few bugs) unless explicitly told xagg==0.3.1
. Perhaps we should make a note of that - I don't think we currently have an environment creation step in the tutorial anyway?
@atrisovic I made the ../data/
edits on the PR #87, since I make other edits to the example step 3 there.
For the warnings and errors from xagg
, here's how things look with the new version:
>>> weightmap = xa.pixel_overlaps(ds_tas, gdf_counties, weights=ds_pop.Population, subset_bbox=False)
creating polygons for each pixel...
lat/lon bounds not found in dataset; they will be created.
calculating overlaps between pixels and output polygons...
success!
>>> aggregated = xa.aggregate(ds_tas, weightmap)
aggregating tas_adj...
aggregating tas_sq...
all variables aggregated to polygons!
>>>
Hello! 👋
Thank you @kls2177 for your thoughtful and detailed review. We really appreciate it. 🙏
Here are the responses to the comments. They are all incorporated and will be merged with the pull request #90
Fixed.
Fixed.
A definition is now added in the same paragraph.
Updated.
The hands-on example is now updated.
Added.
Fixed.
Thank you! A diagram is added.
Fixed.
Fixed.
Both are now $w_p$
Fixed.
Thank you, we updated the wording here to include "meridional" and "zonal" instead of vertical.
Fixed.
We excluded the reference to geogrid
as it is surpassed by Python's rasterio
package.
Fixed.
Fixed.
The wording is updated.
Fixed.
Added.
Added.
Thank you. Added.
Great idea. Added!
Now updated.
Now updated.
Now updated.
Now updated.
@atrisovic
Thanks for these updates. A couple of outstanding comments:
============================== Section: Constructing weather averages within spatial units
Sub-section: Approach 1
It would help if you defined some of your variables: longitude0, longitude1, gridwidth, etc.
============================== Hands-On Exercise, Step 3
During the aggregated = xa.aggregate(ds_tas, weightmap)
step, I am getting the following repeated warning:
RuntimeWarning: invalid value encountered in divide a2 = a2/a2.sum()
@kls2177 Thanks for all of your suggestions. We have now added definitions for the variables under Approach 1 and addressed the warming in xagg
.
For the Approach 1 edits, see ba9bb4180442feb303407031a0293d1d07bae6a1.
This section provides a good introduction to the different formats of socio-economic data, but without too much unnecessary detail. My main comment here is simply the sequencing of material. The Hands-On Exercise, Step 2 makes use of geographical unit data, but this data is not discussed until after. I realize that you want the structure of the tutorial to be such that there is an Exercise at the end of each Chapter, but Step 2 seems out of place. Please consider how you might restructure the Exercises to better align with the flow of the content.
=============================== Section: Generating Geographical Unit Data
============================== Section: Constructing weather averages within spatial units
Sub-section: Approach 1
Sub-section: Approach 2
============================= Section: Weighting Schemes
Sub-section: Aligning Weather and Weighting Grids
Sub-section: Example
Sub-section: Plotting
Section: Hands-On Exercise, Step 3