-
Notifications
You must be signed in to change notification settings - Fork 19
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
Subspace ValueError
with cyclic axis not spanned by data
#756
Comments
ValueError
with presence of cyclic axes not spanned by dataValueError
w/ =>1 cyclic axis not spanned by data
ValueError
w/ =>1 cyclic axis not spanned by dataValueError
w/ >=1 cyclic axis not spanned by data
ValueError
w/ >=1 cyclic axis not spanned by dataValueError
w/ >=1 cyclic axes not spanned by data
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! |
I think that a size 1 axis can be cyclic, such as one with dimension coordinate bounds |
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. |
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
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. |
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 Thanks, |
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 |
ValueError
w/ >=1 cyclic axes not spanned by dataValueError
with cyclic axis not spanned by data
Closed by #758 (my bad not using the keywords properly in the PR comment opening so it automatically shut). |
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:
of:
(specific
t1
,t2
andindex
values not relevant) results in:The text was updated successfully, but these errors were encountered: