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

Explicit error when coordinates are not numercial #190

Merged
merged 8 commits into from
Feb 8, 2023

Conversation

lanougue
Copy link
Contributor

No description provided.

@lanougue
Copy link
Contributor Author

@roxyboy The checks do not seem to be processed. Any idea why ?

xrft/xrft.py Outdated Show resolved Hide resolved
xrft/xrft.py Outdated Show resolved Hide resolved
@roxyboy
Copy link
Member

roxyboy commented Jan 18, 2023

@roxyboy The checks do not seem to be processed. Any idea why ?

@jbusecke Any ideas on why the Pytests are halted..?

@lanougue lanougue requested a review from roxyboy January 20, 2023 18:23
@lanougue
Copy link
Contributor Author

@roxyboy The checks do not seem to be processed. Any idea why ?

@jbusecke Any ideas on why the Pytests are halted..?

Hi @roxyboy ,
Maybe something linked to workflow or rule protection of branches. See link below
https://github.com/orgs/community/discussions/26698

@lanougue
Copy link
Contributor Author

lanougue commented Feb 6, 2023

@rabernat @roxyboy , hello guys, it would be nice to have this new xrft release ! Can you have a look on how to resolve this PR checks ? it could ease the final formatting of these modifications. Thanks !
Cheers

Copy link
Member

@roxyboy roxyboy left a comment

Choose a reason for hiding this comment

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

These edits look good, thanks :) Regarding the stalled tests, I did take a look at the link but I couldn't seem to understand the fix... I've asked an expert so hopefully he'll reply soon.

@cisaacstern
Copy link
Collaborator

Thanks for looping me in here @roxyboy! I've been digging around on SO and the best guess I have so far, based on this post, is that perhaps the fact that this PR's first commit was a Verified commit made via the GitHub web browser is somehow causing problems. One of the suggested solutions in that SO post is to close and then re-open the PR, so I will try that now.

@cisaacstern cisaacstern closed this Feb 6, 2023
@cisaacstern cisaacstern reopened this Feb 6, 2023
@cisaacstern
Copy link
Collaborator

🤔 Ok that did not seem to work...

@cisaacstern
Copy link
Collaborator

@roxyboy and I are coordinating offline on this... he has just removed the requirement in the repo settings that status checks must pass before a PR is merged. From further reading on Stack Overflow it seems this requirement can sometimes interfere with CI running, as we are observing here. I will now close and re-open this PR to re-trigger CI.

@cisaacstern cisaacstern closed this Feb 6, 2023
@cisaacstern cisaacstern reopened this Feb 6, 2023
@jbusecke
Copy link
Contributor

jbusecke commented Feb 6, 2023

Ah ok, sorry then Ill close #193

@cisaacstern
Copy link
Collaborator

Looks like that unblocked us! 🎉

Screen Shot 2023-02-06 at 2 05 04 PM

@roxyboy
Copy link
Member

roxyboy commented Feb 6, 2023

Looks like that unblocked us! 🎉

Screen Shot 2023-02-06 at 2 05 04 PM

Yes, it's looking promising!

@roxyboy
Copy link
Member

roxyboy commented Feb 6, 2023

For the future, should I just keep "Require status checks to pass" unchecked..?

@cisaacstern
Copy link
Collaborator

Ah ok, sorry then Ill close #193

@jbusecke thanks! Takaya looped me in and I didn't realize that you'd opened that PR so just decided to "test in production" 😆 .

@cisaacstern
Copy link
Collaborator

For the future, should I just keep "Require status checks to pass" unchecked..?

@roxyboy I think this is the easiest path forward for now, yes.

@roxyboy
Copy link
Member

roxyboy commented Feb 6, 2023

@lanougue It seems that the tests are failing from xarray.ufunc being deprecated..?

@lanougue
Copy link
Contributor Author

lanougue commented Feb 7, 2023

@roxyboy yes, this is why I opened PR #191. I will do both of these modifications in this PR in order to make the tests to pass.

@lanougue
Copy link
Contributor Author

lanougue commented Feb 8, 2023

@roxyboy I think this is it !

(
is_numeric_dtype(da.coords[d])
or is_datetime64_any_dtype(da.coords[d])
or bool(getattr(da.coords[d][0].item(), "calendar", False))
Copy link
Member

@roxyboy roxyboy Feb 8, 2023

Choose a reason for hiding this comment

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

@lanougue Could you explain what this new condition is for with the calendar...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

xrft.fft can do Fourier Transforms on data with coordinates being of dtype "cftime" (some kind of temporal data with a defined calendar). In the test_xrft.py file, we have some checks with this kind of coordinates with "julian", "365_day", "360_day" type of calendar.
When coordinates are of cftime type, the returned dtype is "object" which is not considered as "numerical" or "datetime" by pandas API. It is thus needed to have some special test to accept this kind of data if we want to pass the checks.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, sounds good. I'll go ahead and merge this :)

@roxyboy roxyboy merged commit f9ab8d2 into xgcm:master Feb 8, 2023
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

4 participants