-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement cftime vectorization as discussed in PR #8322 #8324
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antscloud Nice catch! I have two suggestion to get the test suite running.
@antscloud are you willing to continue this? It would be a good addition. |
cff06d4
to
f28fc7d
Compare
Hi, sorry i completely forgot about this issue, i've implemented the suggestions and all the tests appear to pass, I hope it works in the CI 🤞 |
I didn't check in detail if we have a test that might cover this already and if yes, what the changes are. |
I don't think we currently do, but I think this new benchmark should cover it: #9262. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @antscloud! I think this looks good to me now. Could you add a what's new entry under a new Performance
heading?
3 times faster, not bad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
xarray/coding/times.py
Outdated
dates = np.atleast_1d(dates) | ||
|
||
# Find all the None position | ||
none_position = np.equal(dates, None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy complains here.
Not sure if this is an issue of wrong typing of numpy, or if this is not a recommended way of calling equals. (I think the None is the problem).
Co-authored-by: Michael Niklas <mick.niklas@gmail.com>
The mypy error is still there, not sure how to effectively fix it, maybe use kwargs instead or simply type ignore it. Otherwise this looks good and should get an entry in whats-new! |
As discussed in #8322, here is the test for implementing the vectorization
Only this test seems to fail in
test_coding_times.py
:xarray/xarray/tests/test_coding_times.py
Lines 1061 to 1071 in f895dc1
I don't really understand why though if you have an idea