-
Notifications
You must be signed in to change notification settings - Fork 106
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
Add COPDEM30 #1364
Add COPDEM30 #1364
Conversation
Hello @matthiasdusch! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2022-02-01 21:00:46 UTC |
Thanks so much!!!
Yes, we don't really care about backwards compatibility here (for both the 90 and 30 m products). Cheers! |
fyi: Copernicus EO support responded today, but instead of providing a lookup shapefile like last time they pointed me to a mapping.csv file on their ftp server. This will work as well. |
needs OGGM/oggm-sample-data#12 first and than the hash updated Seems to pull the correct files. No differences on larger glaciers. Visible differences on smaller glaciers. |
Looking good, thanks!!!!! You should be able to change the commit hash now and there seems to be a few pep8 to fix |
@@ -2190,6 +2202,9 @@ def default_dem_source(rgi_id): | |||
sel = cfg.DEM_SOURCE_TABLE[rgi_reg].loc[rgi_id] | |||
for s in ['NASADEM', 'COPDEM', 'GIMP', 'REMA', 'TANDEM', 'MAPZEN']: | |||
if sel.loc[s] > 0.75: | |||
# this can go as soon as 'rgi62_dem_frac.h5' is updated: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmaussion what do you think about this? Not sure when/how/where this 'rgi62_dem_frac.h5' gets updated. Or if this might break somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think this is irrelevant for this PR - I really need to rethink this logic (related: #1269)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thank you! If you are not tired of it a note in whats new
would be welcome! If you have no time no worries I'll merge and do this later
sure, will do. I am just wondering at the moment why these tests take so long or probably timeout? I can't really see at which point they are stuck... can you? |
Not your fault. @TimoRoth and I are investigating, it's probably on the github side |
Thanks a lot! This is awesome. I have hired a student to work on applying this for RGI7, work will start end of February we'll keep you in touch. |
Closes #1362
whats-new.rst
Work in progress. I hope to get a lookup shapefile from Copernicus in the next days. Otherwise this has to be made more or less manually.
Open to discussion: Copernicus offers different releases of both COP30 and 90. Some info can be found here under "Releases": https://spacedata.copernicus.eu/web/cscda/dataset-details?articleId=394198
I guess it makes sense to use the latest version, but it could cause some inconsistencies for people.
note to myself: #961 was the previous PR