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

Fix min_deps_check; revert to support numpy=1.14 and pandas=0.24 #4178

Closed
wants to merge 12 commits into from

Conversation

shoyer
Copy link
Member

@shoyer shoyer commented Jun 25, 2020

Fixes the issue noticed in:
#4175 (comment)

Let's see if this passes CI...

  • Passes isort -rc . && black . && mypy . && flake8

@@ -14,6 +13,17 @@
_USE_BOTTLENECK = False


def normalize_axis_index(data, axis):
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this because I noticed it when manually reverting the quantile change from https://github.com/pydata/xarray/pull/3713/files

Comment on lines -144 to -149
policy_major = req_major
policy_minor = req_minor
policy_published_actual = req_published
for (major, minor), published in reversed(sorted(versions.items())):
if published < policy_published:
break
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely sure why this was broken, but the new logic is significantly simpler so I'm more confident it's correct. In particular, it determines the policy versions without using the currently required versions as defaults.

Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative implement of the original policy that is possibly clearer:

Suggested change
policy_major = req_major
policy_minor = req_minor
policy_published_actual = req_published
for (major, minor), published in reversed(sorted(versions.items())):
if published < policy_published:
break
# "the minor version (X.Y) initially published no more than N months ago"
policy_major, policy_minor = min(
(version for version, date in versions.items() if date > policy_published),
default=(req_major, req_minor),
)

@pep8speaks
Copy link

pep8speaks commented Jun 25, 2020

Hello @shoyer! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-25 03:57:05 UTC

@shoyer shoyer marked this pull request as draft June 25, 2020 04:29
@shoyer
Copy link
Member Author

shoyer commented Jun 25, 2020

Marking this as a draft -- as noted in #4175 (comment), I just misread our current policy.

@andersy005
Copy link
Member

With the three releases (v0.16.0, v0.16.1, v0.16.2) since this PR, is this PR still relevant?

@andersy005 andersy005 added the plan to close May be closeable, needs more eyeballs label Dec 28, 2020
@dcherian
Copy link
Contributor

I think we still want the updates to min_deps_check.py, and then update our min dependencies if still appropriate.

@keewis
Copy link
Collaborator

keewis commented Feb 27, 2021

with the merge of #4907 and #4942 I think we can close this

@keewis keewis closed this Feb 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to close May be closeable, needs more eyeballs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants