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

Cross prime meridian fix #117

Merged
merged 14 commits into from
Feb 3, 2021
Merged

Cross prime meridian fix #117

merged 14 commits into from
Feb 3, 2021

Conversation

ellesmith88
Copy link
Collaborator

@ellesmith88 ellesmith88 commented Feb 2, 2021

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features)
  • Documentation has been added / updated (for bug fixes / features)
  • HISTORY.rst has been updated (with summary of main changes)
  • bumpversion minor has been called on this branch
  • I have added my relevant user information to AUTHORS.md
  • What kind of change does this PR introduce?:
    Bug fix - where the subset request crosses the meridian but the longitudes are all positive, this implements a method to roll the dataset and reassign the coordinates to allow this subset to take place

  • Does this PR introduce a breaking change?:
    I don't think so

  • Other information:
    This also removes pyyaml from the requirements - I added this before the 5.4 of pyyaml version was released on conda to allow the docs to build. This version has now been released so this is no longer needed.

@agstephens
Copy link
Collaborator

@huard @Zeitsperre This is our fix for coping with bounding box subsets where the requested longitude range does not overlap with the longitudes of the data itself. We believe there are no issues from your end because all the changes are dealt with in clisops.ops.subset and we haven't touched clisops.core.subset.

Please shout if you spot any problems with this. We plan to merge it tomorrow morning (UK time).

HISTORY.rst Outdated Show resolved Hide resolved
@Zeitsperre
Copy link
Collaborator

Wow! This is a spotless fix. I'm glad to see that it doesn't roll the data on the core utilities, and it seems to address the concerns you had around this. Nicely done.

Just a thought, but maybe it would be interesting to (in another PR) implement a fix on the core side that turns a bounding box into a shape in the event that it crosses the meridian/edge of the data set and sends that to the subset_shape function? It would be interesting if we could offer both approaches in CLISOPS eventually (rolling and vector-splitting).

Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>
@ellesmith88
Copy link
Collaborator Author

@Zeitsperre Thanks for the review. Sounds like a good idea - I've opened an issue for it #119

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.

None yet

3 participants