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

Fix CI failure due to AZP multiple valid MST #406

Merged
merged 7 commits into from
Nov 26, 2023

Conversation

gegen07
Copy link
Member

@gegen07 gegen07 commented Oct 17, 2023

Summary

  • Read the issue comment here. TLDR; the MST of scipy.sparse graphs generates some fluctuations. Then the proposed fix is presented in util.py. Working with an undirected graph give the same results over the python versions.

  • MST of an unweighted graph is all trees (a path with a single component). Then it would give more than one valid MST using the scipy algorithm. Thus, I computed the distance between centroid polygons based on Queen Weight contiguity to generate a unique valid MST based on Mexico shapefile in test_azp.py.

  • I changed the expected labels in the test to consider all work done in the PR.

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #406 (343ef27) into main (5b7c29c) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #406     +/-   ##
=======================================
+ Coverage   77.6%   77.7%   +0.1%     
=======================================
  Files         27      27             
  Lines       2634    2636      +2     
=======================================
+ Hits        2045    2048      +3     
+ Misses       589     588      -1     
Files Coverage Δ
spopt/region/util.py 63.7% <100.0%> (ø)

... and 2 files with indirect coverage changes

spopt/tests/test_azp.py Outdated Show resolved Hide resolved
spopt/tests/test_azp.py Outdated Show resolved Hide resolved
@jGaboardi
Copy link
Member

@gegen07 This is great work in finding the exact cause of the discrepency here. While the solution here is probably not the most efficient, it certainly does get the job done and makes CI green again (which is a huge accomplishment).

Here's a couple of points to start off the discussion:

  • I computed the distance between centroid polygons based on Queen Weight contiguity to generate a unique valid MST

    • So will this distance method need to be added into AZP itself to ensure user results are reproducible?
  • I am curious how the new Graph class might fit in here.

@knaaptime @martinfleis What are yalls thoughts on this?

@jGaboardi
Copy link
Member

I'm also wondering if functions like generate_initial_sol() might be better place within azp_util.py since it's only used for AZP stuff.

@gegen07 gegen07 changed the title fix: randomly_divide_connected_graph MST problem Fix CI failure due AZP MST error Oct 24, 2023
@gegen07 gegen07 changed the title Fix CI failure due AZP MST error Fix CI failure due AZP multiple valid MST Oct 24, 2023
@jGaboardi
Copy link
Member

jGaboardi commented Oct 24, 2023

New failures due to pysal/libpysal#605, pysal/esda#271. Fix is on the way. --> pysal/esda#272

@gegen07 gegen07 changed the title Fix CI failure due AZP multiple valid MST Fix CI failure due to AZP multiple valid MST Oct 24, 2023
@jGaboardi jGaboardi mentioned this pull request Oct 24, 2023
@gegen07 gegen07 requested a review from ljwolf November 2, 2023 16:46
@@ -747,6 +743,14 @@ def _randomly_divide_connected_graph(adj, n_regions):
f"equal to the number of nodes which is {n_areas}."
)
mst = csg.minimum_spanning_tree(adj)
mst_copy = mst.copy()
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use a classic trick here:

symmetric_mst = (mst + mst.T) > 0

Copy link
Member

Choose a reason for hiding this comment

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

@ljwolf How does this look?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is so smooth!

Copy link
Member

Choose a reason for hiding this comment

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

@gegen07 I know, right? Always some tricks to learn that I have never seen!

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.

I think this is ready to go in my opinion, but we'll wait for @ljwolf' s approval to see if there are any more easy improvements.

@jGaboardi jGaboardi merged commit 5cadae7 into pysal:main Nov 26, 2023
10 checks passed
@jGaboardi jGaboardi mentioned this pull request Dec 3, 2023
25 tasks
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.

3 participants