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 ni and nj as dimensions to be corrected #51

Merged
merged 3 commits into from Jul 16, 2020

Conversation

markusritschel
Copy link
Contributor

@markusritschel markusritschel commented Jul 14, 2020

As a remark:
Sometimes, lon and lat are 2D coordinates depending on ni and nj, respectively. For example,

<xarray.Dataset>
Dimensions:    (bnds: 2, ni: 320, nj: 384, nvertices: 4, time: 1980)
Coordinates:
  * time       (time) object 1850-01-15 12:00:00 ... 2014-12-15 12:00:00
    lon        (nj, ni) float64 ...
    lat        (nj, ni) float64 ...
  * ni         (ni) int32 1 2 3 4 5 6 7 8 9 ... 313 314 315 316 317 318 319 320
  * nj         (nj) int32 1 2 3 4 5 6 7 8 9 ... 377 378 379 380 381 382 383 384
. . .

I added the entries for ni and nj in the rename_dict pretty much in the front of the list since, otherwise, it could lead to unwanted behavior.
The current routine checks only for the first appearance of the key in that list. If ni was placed at the end of the list, the routine would stop at lon (setting the trigger=True and would not even check for ni. In the above example, this would rename the 2D coordinate lon (nj, ni) to x (ni, nj).
The desired behavior, however, is:

    lon        (y, x) float64 ...
    lat        (y, x) float64 ...
  * x          (x) int32 1 2 3 4 5 6 7 8 9 ... 313 314 315 316 317 318 319 320
  * y          (y) int32 1 2 3 4 5 6 7 8 9 ... 377 378 379 380 381 382 383 384

So, one must really keep the order in mind.
Maybe this should be considered when revising the routine at some point.

@codecov
Copy link

codecov bot commented Jul 14, 2020

Codecov Report

Merging #51 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #51   +/-   ##
=======================================
  Coverage   59.92%   59.92%           
=======================================
  Files           6        6           
  Lines         564      564           
=======================================
  Hits          338      338           
  Misses        226      226           
Impacted Files Coverage Δ
cmip6_preprocessing/preprocessing.py 78.94% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1c5f2f...e0892b4. Read the comment docs.

@jbusecke
Copy link
Owner

Ah yes. That is a good point. I was aware of that and aranged these names last, but I agree this is brittle. I will open another issue to track this and think about a way to implement this in a better way.

Copy link
Owner

@jbusecke jbusecke left a comment

Choose a reason for hiding this comment

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

This looks good to me. Dont forget to add an entry in doc/whats-new.rst and then we can get this merged.

Thanks again.

cmip6_preprocessing/preprocessing.py Show resolved Hide resolved
@markusritschel
Copy link
Contributor Author

This looks good to me. Dont forget to add an entry in doc/whats-new.rst and then we can get this merged.

Thanks again.

Sure. Where in the doc file shall I place it? Under v0.2.0 (unreleased) > Internal changes?

Comment on lines +37 to +39
- Add `ni` and `nj` to the `rename_dict` dictionary in _preprocessing.py_ as dimensions to be corrected (:pull:`54`)
By `Markus Ritschel <https://github.com/markusritschel>`_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hope this is on the right place?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes. Awesome. Will merge when the tests have all passed.

@jbusecke
Copy link
Owner

Travis is acting weird. Ill close this and reopen to trigger a new build

@jbusecke jbusecke closed this Jul 16, 2020
@jbusecke jbusecke reopened this Jul 16, 2020
@jbusecke jbusecke closed this Jul 16, 2020
@jbusecke jbusecke reopened this Jul 16, 2020
@jbusecke jbusecke closed this Jul 16, 2020
@jbusecke jbusecke reopened this Jul 16, 2020
@jbusecke
Copy link
Owner

This is quite frustrating. I will wait a bit, to see if there are some outages at either gh or travis that might clear up...
Sorry for the delay.

@markusritschel
Copy link
Contributor Author

This is quite frustrating. I will wait a bit, to see if there are some outages at either gh or travis that might clear up...
Sorry for the delay.

Have you tried doing the CI with Github actions?

@jbusecke
Copy link
Owner

Not yet, but this experience here certainly is a compelling reason to switch...unbelievable. Ill force the merge, since these are very small changes and the PR test went trough.

@jbusecke jbusecke merged commit 8b59a48 into jbusecke:master Jul 16, 2020
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

2 participants