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 longitude_bnds and latitude_bnds to cmip_renaming_dict #300

Merged
merged 14 commits into from
Jun 25, 2024

Conversation

JoranAngevaare
Copy link
Contributor

@JoranAngevaare JoranAngevaare commented Jun 1, 2023

Add longitude_bnds and latitude_bnds to cmip_renaming_dict, this allows renaming for example renaming bounds for 'ScenarioMIP.UA.MCM-UA-1-0.ssp585.r1i1p1f2.Amon.gn.none.tas' .

  • Tests added
  • Passes pre-commit run --all-files
  • Passes pytest
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

@jbusecke
Copy link
Owner

jbusecke commented Jun 7, 2023

Thank you very much for adding these @JoranAngevaare.
I think the testing failure is unrelated (looks like #301). Let me take a crack at that and see if that resolves the CI failures here.
Sorry for the delay.

@jbusecke
Copy link
Owner

jbusecke commented Jun 9, 2023

Ok I am fairly confident that the above issue is fixed by jbusecke/xarrayutils#162, since the upstream-dev tests are now passing. Once v2.0.0 is published on conda-forge I expect the other tests to pass.

@jbusecke
Copy link
Owner

jbusecke commented Jun 9, 2023

Ok I got it all sorted. Thanks for your patience @JoranAngevaare.
Would you mind adding an entry to the docs/whats-new.rst? Once that is done, ill merge this right away!

@JoranAngevaare
Copy link
Contributor Author

@jbusecke thanks a lot for all the fixes! I see that numpy 1.24.0 drop of deprecated np.float caused some issues, the np.zeros(.., float) will be interpreted as np.zeros(.., np.float64)'s which would also have solved that particular problem.

I added a trivial test eff95dc, however, that lead me to notice that bnds is renamed to bnds, I'm not entirely sure if that is a desirable feature. If so, I'll very quickly revert a6a5835.

@jbusecke
Copy link
Owner

that lead me to notice that bnds is renamed to bnds

I am not sure I follow this. I do intent to rename the bounds dimension similarly to others, but I might be missing something in your comment.

@@ -27,7 +27,7 @@ def cmip6_renaming_dict():
"x": ["i", "ni", "xh", "nlon"],
"y": ["j", "nj", "yh", "nlat"],
"lev": ["deptht", "olevel", "zlev", "olev", "depth"],
"bnds": ["bnds", "axis_nbounds", "d2"],
"bnds": ["axis_nbounds", "d2"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jbusecke this is the line I mentioned, so from the cmip6_renaming_dict, you were renaming bnds to bnds. But okay, I'll revert a6a5835 - maybe that is safer.

@jbusecke jbusecke reopened this Jan 25, 2024
@jbusecke
Copy link
Owner

Hi @JoranAngevaare again sorry for the delay (the amount of times I have typed this today 😆). I am not quite following myself from the past TBH. I now completely agree that renaming "bnds" to "bnds" is nonsensical. Sorry for the back and forth. Could you restore that change? Ill get this moving right after the tests pass.

@jbusecke
Copy link
Owner

I would also like to add you to the CITATION.cff for the contributions you made here.

@jbusecke
Copy link
Owner

@JoranAngevaare finally getting around to merging this. Again sorry for the delay. I have opened #358 to track the addition to the CITATION file.

@jbusecke jbusecke merged commit f353598 into jbusecke:main Jun 25, 2024
7 of 8 checks passed
@JoranAngevaare JoranAngevaare deleted the patch-1 branch June 25, 2024 14:20
@JoranAngevaare
Copy link
Contributor Author

hi @jbusecke sorry for the delay on my side - I missed these previous comments.

@JoranAngevaare JoranAngevaare mentioned this pull request Jun 26, 2024
5 tasks
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.

2 participants