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

Cmorize eobs #1554

Merged
merged 21 commits into from
May 7, 2020
Merged

Cmorize eobs #1554

merged 21 commits into from
May 7, 2020

Conversation

mwjury
Copy link
Contributor

@mwjury mwjury commented Feb 28, 2020

@mattiarighi here for E-OBS tas, tasmin, tasmax, pr, psl.
Again with additional monthly means.
I had to fix fix_coords in utilities.py so it can handle asymmetrical longitudes (i.e. not centered at zero).

@mattiarighi
Copy link
Contributor

I'm getting the data.
Could you have a look at the conflicts? Thanks! 👍

Co-Authored-By: Mattia Righi <mattia.righi@dlr.de>
@mwjury
Copy link
Contributor Author

mwjury commented Mar 5, 2020

@mattiarighi The conflict is due to a inconsistency in fix_coordinates, which in its old form can not handle asymmetrical longitudes (i.e. not centered at zero).

@mattiarighi
Copy link
Contributor

There is still a conflit in utilities.py, git cannot merge the branch if this is not solved manually.

@mattiarighi
Copy link
Contributor

@valeriupredoi I need your review on the changes in utilities.py.

@mattiarighi
Copy link
Contributor

@valeriupredoi I tested a few datasets to make sure the changes in utilities.py are back-compatible.
Can you have a look anyway?

@mattiarighi
Copy link
Contributor

@valeriupredoi can I have an approval please! 🍻

@valeriupredoi
Copy link
Contributor

One sec, on it, a bit late in work today - trying to catch a slightly emptier bus to work 🍺

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

I reviewed only the technical bits I assume @mattiarighi you've seen the data provenance stuff. Please have a look at the two important comments that transform longitude points, the other two bits are pythonic stuff 🍺


# Run the cmorization
ver = cfg['attributes']['version']
for res in cfg['attributes']['resolution'].values():
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for res in cfg['attributes']['resolution'].values():
for _, res in cfg['attributes']['resolution'].items():

Copy link
Member

Choose a reason for hiding this comment

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

Why would you want to change this? It looks fine as it is?

@bouweandela
Copy link
Member

@valeriupredoi I noticed that Codacy is complaining about importing private functions (i.e. starting with _) from the utilities.py. Would it be possible to make those functions public in another pull request? Because I guess they're supposed to be used by the cmorizers, right?

@valeriupredoi
Copy link
Contributor

funny, this is not like accessing a protected member from outside the class, but meh, yes I'll do it! 🍺

@valeriupredoi
Copy link
Contributor

valeriupredoi commented May 5, 2020

actually - can we skip it and tell Codacy to go hike? It's an absolute monster to change all the cmorizer scripts, plus, I don't feel like writing documentation for each of these funcs and publishing it, it's something that it's prob best to stay private. What do you think @bouweandela

@bouweandela
Copy link
Member

I agree about not writing the documentation, that would be too much work for very few users, because these functions are only used by people writing cmorizer scripts. However, I think that renaming them without the leading underscore shoudn't be that much work, just use something like

find esmvaltool/cmorizers/ -name '*.py' -exec sed -i 's/_fix_bounds/fix_bounds/' {} +

and carefully look at the git diff before committing to make sure you didn't accidentally change other stuff.

Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

@mattiarighi Looks good to me, can you please test and merge?

@mattiarighi mattiarighi merged commit 1976a80 into master May 7, 2020
@mattiarighi mattiarighi deleted the cmorize_eobs branch May 7, 2020 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants