Migrate to Shapely 2.0+ #22
Conversation
* Remove inheritance from GeometryCollection * Add length property and intersects function
The distance method would raise "RuntimeWarning: invalid value encountered in distance" when acting on a linestring which is a single point. The dwithin method does not.
Where np.where was used, replaced by np.divide with a where argument
We were getting the following error: "The sphinx.configuration key is missing. This key is now required, see our blog post for more information." With the following blog post link: https://about.readthedocs.com/blog/2024/12/deprecate-config-files-without-sphinx-or-mkdocs-config/
TypeError: not all arguments converted during string formatting
|
The issues with the documentation were resolved, see https://solarfactors--22.org.readthedocs.build/en/22/ - but there are quite a few things that still need to be done:
|
|
@kandersolar what do you think? I wasn't aware of your PR #4 when I started working on that, but I see PR #4 is still a draft - besides, I suppose that using Shapely is better than custom implementation of the distance and other methods, isn't it? |
There was a problem hiding this comment.
Amazing @joseph-sch! Thank you very much for this PR, it has been sorely needed for some time.
These changes seem reasonable to me, although unfortunately I'm not familiar enough with shapely to say more than that just by looking at the code. So I want to experiment with it locally before merging. FYI it may be some days before I am able to complete that.
In the meantime, a couple trivial edits below :)
Let's handle these in follow-up PRs, to keep this one manageable. |
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
Of course, these changes need to be checked beyond the unit tests that are implemented within the project - in particular, run tests of pvlib against this version. Thank you for the edits and feedback. Looking forward to your informed opinion after you've run further tests. |
There was a problem hiding this comment.
Comparing this branch with shapely 2.0.6, with the last release of solarfactors (1.5.3) and shapely 1.8.5.post1, I detect no change in runtime, no issue in the solarfactors and pvlib test suites, and identical simulation results when running simple manual tests.
@anomam do you want to review as well? I think I'm happy with this PR.
|
Thanks a lot for this PR @joseph-sch , Shapely v2 also resolved a lot of memory leaks that caused my previous team many headaches so it's a much needed upgrade 🙏 |
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
|
Indeed fixing this issue was a primary motivation for forking solarfactors! It is very nice to see this finally resolved. Thanks again @joseph-sch, and hopefully you have time for more contributions in the future :) |
|
I will fix the linter flags in a follow-up PR, as it looks like this one is not configured to allow maintainers to edit. |
docs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor all changes. Includes link to the GitHub Issue with:issue:`num`or this Pull Request with:pull:`num`. Includes contributor name and/or GitHub username (link with:ghuser:`user`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.This Pull Request updates the codebase of
solarfactorsto support Shapely 2.0+ instead of Shapely < 2, making it compatible with Python 3.12 and later, also allowing upgrade to NumPy 2.0+ (Shapely started support for NumPy 2.0+ in version 2.0.4).The primary change involves refactoring the use of Shapely classes to adopt composition instead of inheritance, aligning with Shapely's updated design principles. Classes in
solarfactorsthat previously inherited from Shapely classes (e.g.,Polygon,Point, etc.) have been refactored to use composition. Shapely objects are now stored as attributes rather than directly subclassed.Updated methods and properties to delegate functionality to the Shapely object (e.g., calling
self.geometry.lengthinstead of directly accessing inherited properties).Confirmed that tests run successfully with Shapely 2.0.6 on Python 3.11, with NumPy 2.2.2.
I don't have a proper benchmark, but I can confirm that the tests are not slower than in the main version.
Along the way, I refreshed the README and also modified code to prevent most (but not all)
RuntimeWarningoccurrences.