Skip to content

Conversation

@vsherratt
Copy link
Contributor

@vsherratt vsherratt commented Oct 8, 2021

🚀 Pull Request

Description

The first concern was that the documentation is not sufficiently specific. I think two key questions users will want answered when they read it are:

  • For coordinates with bounds, how much of a cell must overlap with the range in order to be included?
  • If requesting a full 0-360 degrees splits a cell, which side of the range does the cell go to?

From the code, it looks like the answers are, respectively:

Instead of simply documenting those quirks, I've tried to fix them. I also introduced a threshold argument, so that the answer to "how much overlap" is "configurable" instead of "0".


Consult Iris pull request check list

  • No whatsnew yet - mainly as I wasn't sure what category to put it under.

@vsherratt vsherratt changed the title Intersection Assorted improvements to Cube.intersection Oct 8, 2021
@wjbenfold
Copy link
Contributor

https://scitools-iris.readthedocs.io/en/stable/developers_guide/documenting/whats_new_contributions.html (in case you've not seen it) has some advice on which What's new category to got for - and mentions that you can go for multiple. I'd suggest that maybe adding thresholdis a new feature, and no longer ignoring ignore_bounds is a bug fix?

@vsherratt
Copy link
Contributor Author

Ah! Evidently I missed the "several categories may be used" part...

@trexfeathers trexfeathers self-assigned this Oct 22, 2021
@wjbenfold wjbenfold self-requested a review October 27, 2021 13:29
Copy link
Contributor

@wjbenfold wjbenfold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR, it's great to have new contributors and your changes are not only improvements, they're also significantly easier to understand.

I've popped some comments on specific lines, please let me know if anything isn't clear or if you disagree (you've definitely got more context than me on how this code is used so there may well be reasons I've missed).

Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work @bsherratt and @wjbenfold, thanks so much for taking this on - a big improvement.

@wjbenfold has taken me through the changes and the conversation, and I'm happy to merge. But first @bsherratt please take a look my single comment to make sure you get the recognition you deserve (it helps nurture our community spirit!).

@trexfeathers trexfeathers merged commit 3c087e7 into SciTools:main Nov 4, 2021
@vsherratt vsherratt deleted the intersection branch November 4, 2021 11:15
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