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

unify_time_units() prevents concatenation due to inconsistent dtype #5372

Closed
trexfeathers opened this issue Jul 7, 2023 · 4 comments
Closed
Assignees
Labels
Good First Issue A good issue to take on if you're just getting started with Iris development Type: Bug

Comments

@trexfeathers
Copy link
Contributor

🐛 Bug Report

Thanks to @HelenRJohnson for spotting this.

Using iris.util.unify_time_units() on a CubeList where the time units have an integer dtype prevents the CubeList from being concatenated:

iris.exceptions.ConcatenateError: failed to concatenate into a single cube.
  Dimension coordinates metadata differ: time != time

Explanation

This is because unify_time_units() converts all time units to match the first one. cf-units will not convert something if it is already equal, so the first time unit does not undergo conversion. Meanwhile all the other time units are coerced to dtype="float64", to avoid problems with partial days etc. So the end result is that the first time unit remains an integer dtype while all the others are floats, and this is something that is checked when preparing for concatenation.

Proposed solution

Force all time coordinates to be dtype=float64 at the start of the unify_time_units() process, and advertise this behaviour in the docstring. We should include tests for the new lines, too. I tested this with the below and it fixed my example.

    for cube in cubes:
        for time_coord in cube.coords():
            if time_coord.units.is_time_reference():
+               time_coord.points = time_coord.core_points().astype("float64")
+               time_coord.bounds = time_coord.core_bounds().astype("float64")
                epoch = epochs.setdefault(
                    time_coord.units.calendar, time_coord.units.origin
                )

How To Reproduce

Steps to reproduce the behaviour:

  1. Define a CubeList with multiple Cubes, which can be concatenated together.
    Each Cube should include a time coordinate, using dtype=int for the points.
    Make sure the final Cube's time coordinate has a different epoch to the others (prevents successful concatenation).
  2. Apply iris.util.unify_time_units() to the CubeList.
  3. Attempt CubeList.concatenate_cube() on this CubeList.

Expected behaviour

Should produce a single concatenated Cube.

Environment

  • OS & Version: Red Hat 7.9
  • Iris Version: v3.6.1
@trexfeathers trexfeathers added Type: Bug Good First Issue A good issue to take on if you're just getting started with Iris development labels Jul 7, 2023
@rcomer
Copy link
Member

rcomer commented Jul 7, 2023

This seems familiar… #3213

@github-project-automation github-project-automation bot moved this to 🆕 New - potential tasks in 🦊 Iris v3.7.0 Aug 2, 2023
@pp-mo pp-mo moved this from 🆕 New - potential tasks to 🔖Assigned in 🦊 Iris v3.7.0 Aug 2, 2023
@acchamber acchamber self-assigned this Aug 2, 2023
ESadek-MO pushed a commit that referenced this issue Aug 7, 2023
* Added conversion to dtypes for points and bounds in unify_time_units

* Added whats new entry and updated docstring

* changed docs in response to review
@trexfeathers
Copy link
Contributor Author

Closed by #5412

@github-project-automation github-project-automation bot moved this from 🔖Assigned to 🏁 Done in 🦊 Iris v3.7.0 Aug 7, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Aug 9, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Aug 16, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Aug 23, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Aug 30, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Sep 6, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Sep 13, 2023
@am-thyst
Copy link

Thanks for the fix. It's unfortunate that this issue has been reported for over 10 years.

@trexfeathers
Copy link
Contributor Author

Thanks for the fix. It's unfortunate that this issue has been reported for over 10 years.

Hi @am-thyst, I'm afraid this is the unfortunate reality. The SciTools packages have around 10 regular maintainers (as usual balancing with other responsibilities too), and we prioritise as best we can; this leaves many bugs and good ideas that cannot be worked on with the available resource. I expect we have enough work for 20 maintainers working near full-time, but realistically the current situation is here to stay and we set our expectations accordingly.

I can understand that this might not meet yours and others' needs, and we will always strive to do better, but I want to be honest with you about the situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue A good issue to take on if you're just getting started with Iris development Type: Bug
Projects
No open projects
Status: 🏁 Done
Development

No branches or pull requests

4 participants