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

Don't look for T in sgrid axes #435

Merged
merged 2 commits into from
May 3, 2023
Merged

Don't look for T in sgrid axes #435

merged 2 commits into from
May 3, 2023

Conversation

kthyng
Copy link
Contributor

@kthyng kthyng commented May 3, 2023

cf-xarray is not finding T as a time axis due to a KeyError. This first checks to see if the key is in sgrid_axes before using that.

Closes #434

cf-xarray is not finding T as a time axis due to a KeyError. This first checks to see if the key is in sgrid_axes before using that.
@kthyng
Copy link
Contributor Author

kthyng commented May 3, 2023

@dcherian the errors don't look related as far as I can tell.

@kthyng
Copy link
Contributor Author

kthyng commented May 3, 2023

I'm not sure how other people are using cf-xarray currently because this is a total blocker for me!

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Merging #435 (0f9c7c6) into main (782c9e3) will decrease coverage by 0.02%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main     #435      +/-   ##
==========================================
- Coverage   37.37%   37.35%   -0.02%     
==========================================
  Files          21       21              
  Lines        3914     3916       +2     
  Branches      193      194       +1     
==========================================
  Hits         1463     1463              
- Misses       2258     2259       +1     
- Partials      193      194       +1     
Flag Coverage Δ
mypy 37.35% <33.33%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cf_xarray/accessor.py 35.96% <0.00%> (ø)
cf_xarray/tests/test_accessor.py 29.17% <50.00%> (+0.03%) ⬆️

... and 1 file with indirect coverage changes

@dcherian
Copy link
Contributor

dcherian commented May 3, 2023

This looks right, can you add a test.

Presumably you have an sgrid dataset with the grid_topology variable, Anyone without it is unaffected

@dcherian dcherian changed the title hopeful simple fix for issue Don't look for T in sgrid axes May 3, 2023
@kthyng
Copy link
Contributor Author

kthyng commented May 3, 2023

Ok in modifying a test to check for this I was forced to look into parse_axes some – I'm not familiar with the grid topology stuff at all. But it looks like X, Y, and Z are the only options anyway for key so I put them directly into _get_axis_coord.

Does that work?

@dcherian
Copy link
Contributor

dcherian commented May 3, 2023

Yes I think that's right.

@dcherian dcherian merged commit 6fc7609 into xarray-contrib:main May 3, 2023
This pull request was closed.
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.

time being recognized by not T?
2 participants