Auto qc #4
Auto qc #4
Conversation
There was a problem hiding this comment.
I like the tools and notebooks. The two folium maps (time to peak errors and wse errors) provide good info for modelers. Specific to the code. I'm guessing we'll want to extract the reading from hdf functions into a new PR in rashdf library, go through the review and deploy process, then test importing them here before merging to main? @thwllms can guide us. Here are my suggestions for that:
utils.py
get_domain_names (port directly to rashdf after evaluation duplication/conflicts)
decode_hdf_projection (port directly to rashdf after evaluation duplication/conflicts)
get_projection (port directly to rashdf after evaluation duplication/conflicts)
get_cell_pts (port directly to rashdf after evaluation duplication/conflicts)
get_perimeter (port directly to rashdf after evaluation duplication/conflicts)
hdf_wse.py
package_wse_error_outputs (split up by read hdf parts and processing parts)
read_hdf_wse_errors (split up by read hdf parts and processing parts)
read_hdf_wse_ttp (split up by read hdf parts and processing parts)
wse_error_qc (split up by read hdf parts and processing parts)
wse_ttp_qc (split up by read hdf parts and processing parts)
There was a problem hiding this comment.
Pull the MBI reference out
| # and can be added to the global gitignore or merged into this file. For a more nuclear | ||
| # option (not recommended) you can uncomment the following to ignore the entire idea folder. | ||
| #.idea/ | ||
| # MBI data folder (uncomment after first commit to keep large files out of version control) |
There was a problem hiding this comment.
remove reference to MBI
|
|
||
| Returns | ||
| ------- | ||
| m |
There was a problem hiding this comment.
to be a bit more explicit: "m: folium.Map"
There was a problem hiding this comment.
template files can be removed from PR
There was a problem hiding this comment.
tox file can be removed since we're not using in this project
There was a problem hiding this comment.
Since the .env file is in .gitignore, users won't actually be able to see the template. In this readme, it would be helpful to explain where the .env file lives and its template (so that user can copy and paste)
|
Hoping to look at this in detail soon, but just wanted to point out to @zherbz and @sray014 that both of your PRs include environment stuff which has the potential for conflict. Wondering if it would be better to structure this repo into different folders for each sub-project: |
I think all of these above are already part of
Agree that we need to add the ability to extract WSE values and error to |
There was a problem hiding this comment.
I think this looks pretty good overall as a demo of leveraging RAS HDF data to automatically evaluate certain model results. I generally agree with @wolfpack-daragon's comments. If you want to get the WSE/error values incorporated into rashdf before merging this then I support that. Per my previous comment I think we should probably stick different sets of notebooks into different top-level folders in this repo.
Repo containing notebooks to auto-qc plan HDF files both locally and from S3 for Max WSE Errors and WSE Time-to-Peak