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

Bump numpy versions to match scipy (kinda) #5016

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

hmaarrfk
Copy link
Member

Description

I'm waiting to hear back on the reasons why scipy held back bumping AIX numpy requirements.

scipy/scipy#12940

Hopefully this should help with the failures on the minimum version requirement.

Checklist

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

@hmaarrfk hmaarrfk changed the title WIP: Bump versions to match scipy (kinda) WIP: Bump numpy versions to match scipy (kinda) Oct 12, 2020
@jni
Copy link
Member

jni commented Oct 13, 2020

@hmaarrfk coolio, let us know when this is ready for review/merge.

@hmaarrfk hmaarrfk changed the title WIP: Bump numpy versions to match scipy (kinda) Bump numpy versions to match scipy (kinda) Oct 13, 2020
@hmaarrfk
Copy link
Member Author

rgrommers replied and indicated that the current difference between AIX and non AIX might have been an oversight.

This is ready now.

@jni
Copy link
Member

jni commented Oct 13, 2020

This is ready now.

Travis disagrees. 😂 But actually I don't think it's your fault, I think plotly isn't happy with minimum requirements. @emmanuelle the error is:

  File "doc/examples/segmentation/plot_regionprops.py", line 98, in <module>
    fig = px.imshow(img, binary_string=True)
TypeError: imshow() got an unexpected keyword argument 'binary_string'

I think this feature was only added in the most recent version of plotly, right? Do we need to update our minimum plotly version requirement in the docs requirements?

The other error is also in the docs, a failed download from pivchallenge.org, so I'm approving this.

@hmaarrfk
Copy link
Member Author

The CIs have been really problematic. I thought I had fixed them yesterday with an other PR.

@emmanuelle
Copy link
Member

@jni I opened #5021 re plotly.

In this PR, why do we need to pin exact versions and not just specify a minimum version? Sorry @hmaarrfk if this has already been explained somewhere else.

@hmaarrfk
Copy link
Member Author

Pyproject toml specifies build time dependencies for those building from source using pip tools.

Users on the platforms we support will never interact with this.

However, some users of pip on exotic platforms have asked for it to be kept up to date,and we have obliged.
There, we want to instruct pip to create an environment similar to the ones we have in scikit-image wheels, that is, oldest numpy that we support, before taking the result and installing it in their virtual env satisfying the greater than dependencies we specify.

@jni
Copy link
Member

jni commented Oct 15, 2020

The CIs have been really problematic.

Yeah, let's turn them off, seemed like a good idea at the time but they're just annoying. 🤣

More seriously, I'm happy to merge this as is, or do you want to rebase on master now that #5021 is in?

@emmanuelle
Copy link
Member

@jni in some projects on Github there is an "update branch" button which allows to merge master to the PR branch, and it's very convenient. Maybe it's available only for branches of the same repo and not on a fork? I don't really understand which repos / PRs have this button.

@emmanuelle emmanuelle added this to the 0.18 milestone Oct 15, 2020
@emmanuelle
Copy link
Member

Thanks @hmaarrfk merging!

@emmanuelle emmanuelle merged commit c8e927e into scikit-image:master Oct 15, 2020
@grlee77 grlee77 mentioned this pull request Apr 5, 2021
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