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

Add cook_rgidf() to pretend non-RGI with RGI format #1251

Merged
merged 25 commits into from May 31, 2021

Conversation

Keeptg
Copy link
Member

@Keeptg Keeptg commented May 26, 2021

I add a function cook_rgidf() in utils._funcs to make it easier of using a non-RGI glacier inventory in OGGM.
Will add a tutorial to show how it can be used in future!

  • Tests added/passed
  • Fully documented
  • Entry in whats-new.rst

@pep8speaks
Copy link

pep8speaks commented May 26, 2021

Hello @Keeptg! 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 2021-05-31 13:47:17 UTC

@Keeptg
Copy link
Member Author

Keeptg commented May 28, 2021

Not sure what's the reason the test program failed. Any suggestions wil be appriciated!

@fmaussion
Copy link
Member

The tests fail because the test requires to download input data which is not in the test data, a situation we try to avoid. I think we don't need to test that everything works in your tests, let me have a look at your code later today.

Copy link
Member

@fmaussion fmaussion left a comment

Choose a reason for hiding this comment

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

Thanks! This is looking good. Some minor suggestions to the names, otherwise this looks good!

rgidf = utils.cook_rgidf(cgidf, save_special_columns={'Glc_Long': 'CenLon',
'Glc_Lati': 'CenLat'},
assign_col_values={'Area': cgidf.Glc_Area*1e-6})
gdirs = workflow.init_glacier_directories(rgidf)
Copy link
Member

Choose a reason for hiding this comment

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

Yes so I think you need to stop the test here, and ensure that OGGM could still create the glacier directories properly (that's the main test anyway)

the GeoDataFrame of the user's glacier inventory
region : str
Glacier RGI region code, only usable when set `confirm_region=False`.
The default is '13'.
Copy link
Member

Choose a reason for hiding this comment

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

There should be no default here - it should fail if the region is not provided and cannot be inferred automatically

Glacier RGI region code, only usable when set `confirm_region=False`.
The default is '13'.
version : str
Glacier inventory version code. The default is '60'.
Copy link
Member

Choose a reason for hiding this comment

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

Explain that this is necessary to generate the RGI Ids


# If there are specifical column in the original glacier inventory we want to keep
if save_special_columns is not None:
for col in save_special_columns.keys():
Copy link
Member

Choose a reason for hiding this comment

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

loop like:
for key, val in dict.items()

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! Didn't know this before.

following the glacier order.
id_suffix : str or None
Add a suffix to the glacier id. The default is None, no suffix
save_special_columns : dict or None
Copy link
Member

Choose a reason for hiding this comment

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

could this be only one argument (assign_col_values) where the right column name in the RGI format is already used? The users can change the name themselves if they want to, I'm not sure this dict here is very useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK! assign_col_values argument was deleted!

@Keeptg
Copy link
Member Author

Keeptg commented May 29, 2021

There are still two error reported.
Don't know how to deal with them......

@fmaussion
Copy link
Member

Thanks @Keeptg ! sorry this took so long, excellent work!

@fmaussion fmaussion merged commit 8339947 into OGGM:master May 31, 2021
@Keeptg Keeptg deleted the cook_rgidf branch June 1, 2021 02: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.

None yet

3 participants