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

Add predefined locations to coverage models #231

Merged
merged 4 commits into from
Apr 24, 2022

Conversation

gegen07
Copy link
Member

@gegen07 gegen07 commented Apr 17, 2022

Based on ideas raised in discussion #218.
I'm trying to implement @jGaboardi idea, for instance.

@gegen07 gegen07 added enhancement New feature or request locate labels Apr 17, 2022
@gegen07 gegen07 self-assigned this Apr 17, 2022
@codecov
Copy link

codecov bot commented Apr 17, 2022

Codecov Report

Merging #231 (69df37c) into main (76dd905) will increase coverage by 0.4%.
The diff coverage is 90.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #231     +/-   ##
=======================================
+ Coverage   65.8%   66.1%   +0.4%     
=======================================
  Files         23      23             
  Lines       2535    2554     +19     
  Branches     552     559      +7     
=======================================
+ Hits        1667    1689     +22     
+ Misses       787     779      -8     
- Partials      81      86      +5     
Impacted Files Coverage Δ
spopt/locate/base.py 96.1% <77.8%> (-1.8%) ⬇️
spopt/locate/coverage.py 100.0% <100.0%> (ø)
spopt/_version.py 33.8% <0.0%> (+1.5%) ⬆️

@jGaboardi
Copy link
Member

This is a very nice start @gegen07!

@jGaboardi
Copy link
Member

I just realized that we are not assigning decision variable labels for plotting, but only using the range (see plot_results in the MCLP notebook). By doing this we have the same labels for the solution facilities in "cost matrix" and "geodataframe" examples, though the selection set is different. In order to ensure comparable results (and confirm the preselected facilities are being adhered to) we need to label the facilities from a static index (e.g. decision variable label from the pulp model) and also ensure a 1:1 color scheme (e.g.

@gegen07
Copy link
Member Author

gegen07 commented Apr 19, 2022

In the MCLP notebook we have different labels in the legend.
We are not getting the label from the pulp model to create the facility-client matrix. When do you say create a static index you mean to get the labels from pulp, like another method to get these labels, or just assign a 1:1 dictionary in the plot_results method using the indexes of the facility-client matrix?

@jGaboardi
Copy link
Member

In the MCLP notebook we have different labels in the legend.
We are not getting the label from the pulp model to create the facility-client matrix. When do you say create a static index you mean to get the labels from pulp, like another method to get these labels, or just assign a 1:1 dictionary in the plot_results method using the indexes of the facility-client matrix?

ah! You are right about that, I completely misread the legend. My fault. Though it is still good to have a defined color map so that a specific color is tied to a specific facility. Then the two plots in the notebook are easier to compare.

@gegen07 gegen07 marked this pull request as ready for review April 21, 2022 22:46
@gegen07 gegen07 changed the title [WIP] add predefined locations to coverage models Add predefined locations to coverage models Apr 22, 2022
Copy link
Member

@jGaboardi jGaboardi left a comment

Choose a reason for hiding this comment

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

This is unbelievably great work @gegen07. Let's also try to get a reviews from Qunshan and Levi if they have time. But it looks great to me.

@jGaboardi jGaboardi requested a review from ljwolf April 22, 2022 22:09
@jGaboardi
Copy link
Member

@qszhao Got time for a review?

@jGaboardi
Copy link
Member

I think this is probably good to go and we can go ahead and merge. 🎉

@jGaboardi jGaboardi merged commit 1929cfe into pysal:main Apr 24, 2022
@qszhao
Copy link

qszhao commented Apr 24, 2022

Thanks @jGaboardi and @gegen07! Great to have this question sorted out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request locate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants