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

Implement cftime vectorization as discussed in PR #8322 #8324

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

antscloud
Copy link
Contributor

As discussed in #8322, here is the test for implementing the vectorization

Only this test seems to fail in test_coding_times.py :

@requires_cftime
@pytest.mark.parametrize("freq", ["U", "L", "S", "T", "H", "D"])
def test_encode_decode_roundtrip_cftime(freq) -> None:
initial_time = cftime_range("0001", periods=1)
times = initial_time.append(
cftime_range("0001", periods=2, freq=freq) + timedelta(days=291000 * 365)
)
variable = Variable(["time"], times)
encoded = conventions.encode_cf_variable(variable)
decoded = conventions.decode_cf_variable("time", encoded, use_cftime=True)
assert_equal(variable, decoded)

I don't really understand why though if you have an idea

@dcherian dcherian changed the title Implement vectorization as discussed in PR #8322 Implement cftime vectorization as discussed in PR #8322 Oct 17, 2023
@Illviljan Illviljan added the run-benchmark Run the ASV benchmark workflow label Oct 17, 2023
Copy link
Contributor

@kmuehlbauer kmuehlbauer left a 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.

xarray/coding/times.py Outdated Show resolved Hide resolved
xarray/coding/times.py Show resolved Hide resolved
xarray/coding/times.py Outdated Show resolved Hide resolved
@headtr1ck
Copy link
Collaborator

@antscloud are you willing to continue this? It would be a good addition.

@antscloud antscloud force-pushed the vectorization_of_encode_cftime branch from cff06d4 to f28fc7d Compare July 14, 2024 15:27
@antscloud
Copy link
Contributor Author

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 🤞

@headtr1ck
Copy link
Collaborator

BENCHMARKS NOT SIGNIFICANTLY CHANGED.

I didn't check in detail if we have a test that might cover this already and if yes, what the changes are.

@spencerkclark
Copy link
Member

I don't think we currently do, but I think this new benchmark should cover it: #9262.

Copy link
Member

@spencerkclark spencerkclark left a 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?

@headtr1ck
Copy link
Collaborator

headtr1ck commented Jul 20, 2024

| Change | Before [91a52dc] | After [3512bee] | Ratio | Benchmark (Parameter) |
|----------|----------------------|---------------------|---------|-----------------------------------------------------------|
| - | 139±0.9ms | 42.3±0.7ms | 0.3 | coding.EncodeCFDatetime.time_encode_cf_datetime('noleap') |

3 times faster, not bad

Copy link
Collaborator

@headtr1ck headtr1ck left a 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 Show resolved Hide resolved
dates = np.atleast_1d(dates)

# Find all the None position
none_position = np.equal(dates, None)
Copy link
Collaborator

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).

@headtr1ck
Copy link
Collaborator

headtr1ck commented Oct 14, 2024

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!

xarray/coding/times.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-benchmark Run the ASV benchmark workflow topic-cftime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants