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.

Sign up for GitHub

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

Jump to bottom

Subspace ValueError with cyclic axis not spanned by data #756

Closed
sadielbartholomew opened this issue Apr 5, 2024 · 7 comments
Closed

Subspace ValueError with cyclic axis not spanned by data #756

sadielbartholomew opened this issue Apr 5, 2024 · 7 comments
Assignees
@sadielbartholomew
Labels
bug Something isn't working

Comments

@sadielbartholomew
Copy link
Member

sadielbartholomew commented Apr 5, 2024 •
edited

A ValueError: tuple.index(x): x not in tuple is raised when performing a subspace on a Field which should logically work, in the case that there is at least one domain axis construct not spanned by the data which is cyclic.

An example failure is shown below, which was raised in our VISION end-to-end script when it previously worked fine.

This seems to result from an issue introduced in #745, since a git blame shows that line has been changed there and the same subspace worked witout issue before that PR, corresponding to the time I noticed the VISION script throw the error. In which case, apologies I missed this during code review for that PR.

Fix to follow.

Example failure

For a subspace attempt on the following field:

Field: id%UM_m01s51i010_vn1105 (ncvar%UM_m01s51i010_vn1105)
-----------------------------------------------------------
Data            : id%UM_m01s51i010_vn1105(time(5), ncdim%obs(11160)) 1
Cell methods    : time(5): point
Dimension coords: time(5) = [2017-07-03 11:00:00, ..., 2017-07-03 15:00:00] standard
Auxiliary coords: time(ncdim%obs(11160)) = [2017-07-03 11:15:07, ..., 2017-07-03 14:21:06] standard
                : altitude(ncdim%obs(11160)) = [2577.927001953125, ..., 151.16905212402344] m
                : air_pressure(ncdim%obs(11160)) = [751.6758422851562, ..., 1006.53076171875] hPa
                : latitude(ncdim%obs(11160)) = [52.56147766113281, ..., 52.0729866027832] degree_north
                : longitude(ncdim%obs(11160)) = [0.3171832859516144, ..., -0.6249311566352844] degree_east

of:

s0 = m.subspace(
        **{
            f.auxiliary_coordinate("T", key=True): cf.wi(cf.dt(t1), cf.dt(t2)),
            f.dimension_coordinate("T", key=True): [index],
        }
    )

(specific t1, t2 and index values not relevant) results in:

Traceback (most recent call last):
  File "/home/slb93/vision-twine/local-work/cf-vision-flight-e2e.py", line 314, in <module>
    s0 = m.subspace(
         ^^^^^^^^^^^
  File "/home/slb93/git-repos/cf-python/cf/subspacefield.py", line 289, in __call__
    raise error
  File "/home/slb93/git-repos/cf-python/cf/subspacefield.py", line 284, in __call__
    out = field[indices]
          ~~~~~^^^^^^^^^
  File "/home/slb93/git-repos/cf-python/cf/field.py", line 446, in __getitem__
    data_axes.index(axis) for axis in new.cyclic() ###if axis in data_axes
    ^^^^^^^^^^^^^^^^^^^^^
ValueError: tuple.index(x): x not in tuple
@sadielbartholomew sadielbartholomew added the bug Something isn't working label Apr 5, 2024
@sadielbartholomew sadielbartholomew self-assigned this Apr 5, 2024
@sadielbartholomew sadielbartholomew changed the title Field subspace ValueError with presence of cyclic axes not spanned by data Field subspace ValueError w/ =>1 cyclic axis not spanned by data Apr 5, 2024
@sadielbartholomew sadielbartholomew mentioned this issue Apr 5, 2024
Closed
@sadielbartholomew sadielbartholomew changed the title Field subspace ValueError w/ =>1 cyclic axis not spanned by data Field subspace ValueError w/ >=1 cyclic axis not spanned by data Apr 5, 2024
@sadielbartholomew sadielbartholomew changed the title Field subspace ValueError w/ >=1 cyclic axis not spanned by data Field subspace ValueError w/ >=1 cyclic axes not spanned by data Apr 5, 2024
@sadielbartholomew
Copy link
Member Author

Actually, looking over my example and the logic (and I should stop now, as it's past the end of the working week...) something weird is going on, probably elsewhere previously in the code, since (quoting from the docs since I went there for confirmation) "The data array of the field construct spans all the domain axis constructs with the possible exception of size one domain axis constructs" but I don't think a size-one axis can (/should) be cyclic. So we shouldn't need the catch that seems to make the fix, as per my draft PR #757, since contextually it shouldn't be needed.

To be investigated on Monday - I'll work out the real culprit!

@davidhassell
Copy link
Collaborator

a size-one axis can (/should) be cyclic.

I think that a size 1 axis can be cyclic, such as one with dimension coordinate bounds [[0, 360]] degrees_east.

@sadielbartholomew
Copy link
Member Author

Oh I see - thanks for the example, that makes sense as a cyclic size-1 axis. In which case, the one-line fix in #757 is necessary? Was working on VISION work but I will go back and update that PR and open it once I sort a little testing to cover the case which errored.

@sadielbartholomew sadielbartholomew mentioned this issue Apr 9, 2024
Closed
@sadielbartholomew
Copy link
Member Author

After more investigation and failure to find a minimal working example, I have done a write up in #758 to outline how I believe the underlying error is introduced because a domain axis is not removed from the cyclic() set when it should be, but I also think from the evidence above, namely conformation that:

a size 1 axis can be cyclic

that adding in a tweak to the logic as I have suggested in the PR #757 will make the present logic more robust, so is a good idea, even if it isn't the fix for the underlying issue at hand. What do you think, @davidhassell? Happy to close #757 though if you think it unsuitable given my investigations from #758.

@davidhassell
Copy link
Collaborator

Hi Sadie,

I think your investigation in #758 is on point, thanks. Might it not be better, however, to fix the bug that left the rogue domain axis there in first place, rather than to cover it up with #757?

I think that you've shown convincingly that the source of the problem is del_construct not properly tidying up after itself (here, I reckon: https://github.com/NCAS-CMS/cf-python/blob/main/cf/regrid/regrid.py#L2995), so perhaps the answer could be to overload the inherited cfdm.Field.del_construct (in cf/mixin/fielddomain.py) to check the cyclic axes when it deletes a domain axis construct?

Thanks,
David

@sadielbartholomew
Copy link
Member Author

Thanks for your thoughts, David. OK, so I close the currently-attached PR #757 and we make a fix related more directly to #758. That sounds good to me, assuming as I believe you are implying that, unless there is a bug such as #758 which means something wrong gets through, there is never a case whereby, for org_cyclic = [data_axes.index(axis) for axis in new.cyclic()], there can be an axis in cyclic() which isn't in .get_data_axes()?

@sadielbartholomew sadielbartholomew changed the title Field subspace ValueError w/ >=1 cyclic axes not spanned by data Subspace ValueError with cyclic axis not spanned by data Apr 22, 2024
@sadielbartholomew sadielbartholomew mentioned this issue Apr 22, 2024
Merged
@sadielbartholomew
Copy link
Member Author

Closed by #758 (my bad not using the keywords properly in the PR comment opening so it automatically shut).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sadielbartholomew sadielbartholomew

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Prevent Field subspace ValueError from cyclicity check sadielbartholomew/cf-python
2 participants
@davidhassell @sadielbartholomew