GerryChain icon indicating copy to clipboard operation
GerryChain copied to clipboard

Add split pairs metric

Open jacobwachspress opened this issue 2 years ago • 5 comments

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.

jacobwachspress avatar Apr 24 '22 19:04 jacobwachspress

Codecov Report

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

:exclamation: 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%) :arrow_down:

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.

codecov-commenter avatar Apr 24 '22 19:04 codecov-commenter

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.

InnovativeInventor avatar May 07 '22 21:05 InnovativeInventor

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

jacobwachspress avatar May 14 '22 17:05 jacobwachspress

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!

pizzimathy avatar May 25 '22 18:05 pizzimathy

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

jacobwachspress avatar Sep 04 '22 19:09 jacobwachspress