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

Update/cleanup/standardize notebooks #288

Merged
merged 10 commits into from
Oct 29, 2022
Merged

Conversation

jGaboardi
Copy link
Member

@jGaboardi jGaboardi commented Oct 23, 2022

This is a relatively large PR that touches all notebooks. It cleans up, modernizes, and standardizes both notebook sets, region and locate, but mainly focuses on highlighting and consolidating the work by @gegen07 from GSoC 2021 and @erinrolson from GSoC 2022 (awesome work from both!)

Also overhauls spopt.locate.utils.simulate_geo_points() for robustness and adds tests.

@jGaboardi jGaboardi requested a review from gegen07 October 23, 2022 05:53
@jGaboardi jGaboardi self-assigned this Oct 23, 2022
@jGaboardi jGaboardi mentioned this pull request Oct 23, 2022
@jGaboardi
Copy link
Member Author

It also cleans up and adds the two notebooks to the docs that @TimMcCauley contributed (we forgot to add those in previously). Thanks again, @TimMcCauley!

@TimMcCauley
Copy link
Contributor

It also cleans up and adds the two notebooks to the docs that @TimMcCauley contributed (we forgot to add those in previously). Thanks again, @TimMcCauley!

Happy to! If you have further ideas, I'd be happy to whip something 🆙

@jGaboardi
Copy link
Member Author

@TimMcCauley I am happy to review anything you'd be willing to contribute!

@jGaboardi
Copy link
Member Author

jGaboardi commented Oct 27, 2022

@gegen07 Any chance of getting a review of this PR for merge? Not trying to rush if you don't have the time. Also, there are no functionality changes in this PR, mostly just sprucing up of notebooks, etc.

*There are minor functionality changes in simulate_geo_points().

@gegen07
Copy link
Member

gegen07 commented Oct 27, 2022

I'll review it tomorrow. Sorry for delay, I've been quite busy this week.

@jGaboardi
Copy link
Member Author

I'll review it tomorrow. Sorry for delay, I've been quite busy this week.

No problem! Thanks so much!

@jGaboardi
Copy link
Member Author

Addressing #289 within this PR, as well (@martinfleis)

@jGaboardi jGaboardi requested a review from martinfleis October 27, 2022 22:31
@jGaboardi
Copy link
Member Author

OK, @martinfleis I've found something for you on the shapely==2.0b1 front. Here's a MWE to reproduce:

Python 3.10.6 | packaged by conda-forge | (main, Aug 22 2022, 20:41:54) [Clang 13.0.1 ]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.5.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import numpy
   ...: import shapely
   ...: from shapely.geometry import Point
   ...: shapely.__version__
Out[1]: '2.0b1'

In [2]: x, y = numpy.array([1]), numpy.array([1])

In [3]: x, y
Out[3]: (array([1]), array([1]))

In [4]: Point(x, y)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In [4], line 1
----> 1 Point(x, y)

File ~/miniconda3/envs/py310_shapely_20b1/lib/python3.10/site-packages/shapely/geometry/point.py:77, in Point.__new__(self, *args)
     74     geom = shapely.points(np.array(args))
     76 if not isinstance(geom, Point):
---> 77     raise ValueError("Invalid values passed to Point constructor")
     78 return geom

ValueError: Invalid values passed to Point constructor

Two singular value numpy.array objects were previously valid inputs to construct a Point. Is this a regression in shapely or something that should change from the user's side?

@martinfleis
Copy link
Member

@jGaboardi good catch! That'll be a regression on shapely side. I'll report it there.

@jGaboardi
Copy link
Member Author

@gegen07 We can ignore the failing 310-DEV_shapely20b1 tests for now.

@jGaboardi
Copy link
Member Author

ValueError: Invalid values passed to Point constructor being addressed in shapely/shapely#1590

Copy link
Member

@gegen07 gegen07 left a comment

Choose a reason for hiding this comment

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

@jGaboardi LGTM!
Just one comment that I didn't get!

spopt/locate/util.py Show resolved Hide resolved
@jGaboardi jGaboardi merged commit 826e3c3 into pysal:main Oct 29, 2022
@jGaboardi jGaboardi deleted the rerun_notebooks branch October 29, 2022 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P-Median Problem Tutorial Binder example update docs for new location models
4 participants