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

var_name in AuxCoord created by trajectory.interpolate() #3718

Merged
merged 3 commits into from
Sep 12, 2020

Conversation

SimonPeatman
Copy link
Contributor

When iris.analysis.trajectory.interpolate() creates AuxCoords for the coordinates interpolated along, it doesn't give them a var_name. This causes an error if the coordinates differ in var_name only, because Iris doesn't like having two identical coordinates on the same Cube.

Use case: I had a Cube which was a 2D histogram, with the two coordinates being some diagnostic on day n and the same diagnostic on day n+1, so they were the same thing physically and therefore differed in var_name only. When I tried to use trajectory.interpolate() it crashed with "ValueError: Duplicate coordinates are not permitted.".

@rcomer
Copy link
Member

rcomer commented Aug 28, 2020

Hi @SimonPeatman, thanks for contributing! I can't see a reason why the var_name shouldn't be copied across, along with the rest of the metadata. It looks like this hasn't changed since the initial commit to GitHub.

You just have one failing test because the expected output doesn't have the var_names, so you'll need to fix this directly by updating lines 80 and 90 of the relevant cml file to match the new expected behaviour.

Also, I think this is technically an incompatible change since there is a small chance it could break someone's code. E.g. if var_names are currently being auto-generated to something else when the output is saved to NetCDF and the subsequent analysis expects those auto-generated var_names. Incompatible changes are allowed if we squeeze this in in time for the imminent Iris 3 release, but should have a bullet point in the relevant section of the whatsnew document.

@rcomer rcomer self-assigned this Aug 28, 2020
@SimonPeatman
Copy link
Contributor Author

Thanks @rcomer - the tests are now all passing. My branch doesn't have the latest.rst document (it must have been created recently). Is there something I can do to update my branch with the latest commits from SciTools/iris so I can edit that document? (My knowledge of GitHub isn't quite up to scratch I'm afraid!)

@trexfeathers
Copy link
Contributor

trexfeathers commented Sep 11, 2020

Is there something I can do to update my branch with the latest commits from SciTools/iris so I can edit that document?

@SimonPeatman If you can wait until #3838 and #3843 have gone in, they should significantly clarify what is expected with "What's New" contributions. If you're in a hurry by all means get started now, but please have a look at this preview of the planned changed documentation on "What's New" contributions

@rcomer
Copy link
Member

rcomer commented Sep 11, 2020

Hi @SimonPeatman, thanks for your update. You can pick up all the changes since you made your branch by rebasing it. Assuming you are in your working copy directory, it should just be a case of

git fetch upstream
git rebase upstream/master

then, when you next push to github, you’ll likely get an error to do with “fast-forward updates” which you can get around with the -f option.

Sorry, I should have realised you’d need this info before.

@SimonPeatman SimonPeatman force-pushed the trajectory-interpolate-var-name branch from b2b0301 to 8a0ddfa Compare September 11, 2020 20:32
@SimonPeatman
Copy link
Contributor Author

Thanks both, this should be done now. I followed the advice in the Contributing a "What's New" entry page which @trexfeathers linked to, even though that doesn't quite follow the format of other entries in the latest.rst file (e.g., including my name and a link to my GitHub page).

@rcomer
Copy link
Member

rcomer commented Sep 12, 2020

Thanks @SimonPeatman, I’ve just had a read through @trexfeathers’ link too as the new style is also new to me! I believe there is a plan to update the existing whatsnew entries to the new style before the release.

This all seems to be in place. Welcome to the contributors list!

@rcomer rcomer merged commit a338ca5 into SciTools:master Sep 12, 2020
@rcomer rcomer added this to the v3.0.0 milestone Sep 12, 2020
tkknight added a commit to tkknight/iris that referenced this pull request Sep 15, 2020
* master:
  whatsnew - using the author github id (SciTools#3849)
  Bring SciTools#3804 whatsnew in line with format changes from SciTools#3838. (SciTools#3847)
  "What's New" credit and referencing (SciTools#3838)
  Allow passing None for all coord-system optional args. (SciTools#3804)
  var_name in AuxCoord created by trajectory.interpolate() (SciTools#3718)
@SimonPeatman SimonPeatman deleted the trajectory-interpolate-var-name branch September 22, 2020 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants