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

Restrict shapely dependency to <=2 #130

Merged
merged 4 commits into from
Jan 11, 2022

Conversation

kandersolar
Copy link
Contributor

Closes #126.

I wonder if we should think about releasing 1.5.2 soon? Besides these deprecation warnings and the tilt=0 issue from #125, it would be nice for the latest pvfactors version to allow pvlib==0.9.0 on installation. Someone asked about this on the pvlib google group: https://groups.google.com/g/pvlib-python/c/hNaNsM1pNSw

@kandersolar
Copy link
Contributor Author

Hi @campanelli-sunpower, just a bump, especially about cutting a new release that doesn't forbid the latest pvlib :)

Copy link
Contributor

@campanelli-sunpower campanelli-sunpower left a comment

Choose a reason for hiding this comment

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

Because pvfactors is a library, I think we should allow upgrading of Shapely until (but not including) v2, at which point the warnings will become failures.

requirements.txt Outdated Show resolved Hide resolved
docs/sphinx/whatsnew/v1.5.2.rst Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@campanelli-sunpower campanelli-sunpower left a comment

Choose a reason for hiding this comment

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

Thanks @kanderso-nrel .

@kandersolar
Copy link
Contributor Author

I suspect the test failures are caused by the numpy change discussed in pvlib/pvlib-python#1369

If that's true (need to verify it locally), I'd advocate for the same fix: weaken the test tolerance a bit to allow for the variation from numpy

@campanelli-sunpower
Copy link
Contributor

@kanderso-nrel Looks like some small numerical drift on the newer Pythons. I think it would be ok to widen the tolerance slightly.

Note the recommendation here to not use numpy.testing.assert_almost_equal:

It is recommended to use one of assert_allclose, assert_array_almost_equal_nulp or assert_array_max_ulp instead of this function for more consistent floating point comparisons.

@kandersolar
Copy link
Contributor Author

@campanelli-sunpower Shall I update this PR with those changes?

@campanelli-sunpower
Copy link
Contributor

@kanderso-nrel Please do so if you are ok with that. It would keep the review sequence more straightforward. Thanks.

Copy link
Contributor

@campanelli-sunpower campanelli-sunpower 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 updating these test methods too.

@campanelli-sunpower campanelli-sunpower changed the title Restrict shapely dependency to <=1.7.1 Restrict shapely dependency to <=2 Jan 11, 2022
@campanelli-sunpower campanelli-sunpower merged commit 91c7657 into SunPower:master Jan 11, 2022
@kandersolar kandersolar deleted the shapely171 branch January 11, 2022 23:58
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.

Importing pvfactors causes ShapelyDeprecationWarnings for shapely >= 1.8
2 participants