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 split pairs metric #394

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jacobwachspress
Copy link

Updating locality_split_scores.py with split pairs metric (see this github repo and paper for more details).

By the way, is locality_split_scores.py outdated? It looks like there is only support for CountySplit in updaters. Also the Shannon entropy metric seems to be calculated as though a VTD is a person. Is there code somewhere else that has updated metrics? If so, I am happy to contribute there.

update localty_split_scores.py with split pairs metric
shortened some lines
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2022

Codecov Report

Merging #394 (930209e) into main (18cde80) will decrease coverage by 1.03%.
The diff coverage is 8.69%.

❗ Current head 930209e differs from pull request most recent head 0a2c39d. Consider uploading reports for the commit 0a2c39d to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #394      +/-   ##
==========================================
- Coverage   87.83%   86.80%   -1.04%     
==========================================
  Files          37       37              
  Lines        1735     1758      +23     
==========================================
+ Hits         1524     1526       +2     
- Misses        211      232      +21     
Impacted Files Coverage Δ
gerrychain/updaters/locality_split_scores.py 88.70% <8.69%> (-11.30%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18cde80...0a2c39d. Read the comment docs.

@InnovativeInventor InnovativeInventor self-requested a review May 3, 2022 20:44
Copy link
Member

@InnovativeInventor InnovativeInventor 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 the PR!

By the way, is locality_split_scores.py outdated? It looks like there is only support for CountySplit in updaters. Also the Shannon entropy metric seems to be calculated as though a VTD is a person. Is there code somewhere else that has updated metrics? If so, I am happy to contribute there.

I don't think so. FYI, most of the metrics that we use are implemented here: https://github.com/mggg/evaltools (docs). I'm not too familiar with the Shannon entropy metric, but maybe @apizzimenti or @jenni-niels knows more about that.

The PR looks good to me, so I'm approving it (although it might be more appropriate for this code to live in evaltools).

@InnovativeInventor InnovativeInventor changed the title add split pairs metric Add split pairs metric May 7, 2022
@InnovativeInventor
Copy link
Member

Also, if you get the chance to add unit tests for this metric, that'd be good (but not necessary, I think). Feel free to merge once you see this.

@jacobwachspress
Copy link
Author

Also, if you get the chance to add unit tests for this metric, that'd be good (but not necessary, I think). Feel free to merge once you see this.

It says "Only those with write access to this repository can merge pull requests." Need you to do that

@pizzimathy
Copy link
Member

pizzimathy commented May 25, 2022

Hey @jacobwachspress — we're taking some time to review your PR, and are slotting it in with a number of other internal reviews we'll be conducting in the near future. In the meantime, would you be able to write us some unit tests and add them to your PR? Thanks!

add split pairs metric to unit test
@jacobwachspress
Copy link
Author

jacobwachspress commented Sep 4, 2022

Sorry for the delay... I added analogous unit tests to the file where the other split scores are tested. However, I am having issues with the testing. First of all, it says "First-time contributors need a maintainer to approve running workflows." (I think this is because I am trying to change a file that does the unit tests....?) Second of all, the testing environment does not seem to be loading properly, instead terminating with this error:

Installed /root/conda/lib/python3.8/site-packages/contourpy-1.0.5-py3.8-linux-x86_64.egg
error: shapely 2.0a1 is installed but shapely<2,>=1.7 is required by {'geopandas'}

I would be surprised if the errors are related, but could you try hitting "approve and run" to see if the tests pass? My changes are so minor that I would be very surprised if they broke anything.

EDIT: I think the shapely version error might just be an existing issue with the test_and_deploy workflow? And I might just be stumbling on it now because this is the first time my PR was tested via that workflow, as I modified a unit test

@pjrule pjrule added the contrib label Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants