TestAZP.test_azp_basic_from_w CI failure
#386
Assignees
Labels
Comments
This was referenced Jun 27, 2023
|
Recording package versions for posterity. .ci/311.yml Ubuntu (last known pass).ci/311.yml Ubuntu (current fail).ci/38.yml Ubuntu (pass) |
|
Seems likely an interplay between
|
This was referenced Jul 11, 2023
|
@jGaboardi I dug into that, but I couldn't get the error. I found that there's a difference between scipy versions 1.10.1 and 1.11.1, when using the 1.10.1 version the result is Furthermore, changing numpy versions and keeping the scipy the same doesn't change anything on AZP results, neither on random choice. Maybe the error is related to the random number generation, but this is so weird... |
This was referenced Oct 3, 2023
|
@martinfleis Here. |
|
A question without me looking at the code. Is the new result wrong? Or just different due to effects of randomness? |
|
That is unknown to me, and a great question. I am not an expert in regionalization. @sjsrey @knaaptime @weikang9009 Did any of you originally code up the AZP algo? |
|
Is there a reference implementation in R we use use to check what they get? |
|
From @gegen07's debugging several back, it does seem to be some difference in how random states are being handled. If that's true then the new result is valid and we can update the test. However, we need to be certain... The some implementations to potentially test against: Seems like all implementations have been developed by within our cluster of researchers... (pun intended) |
This was referenced Oct 14, 2023
|
@jGaboardi and @martinfleis, I noticed, after tracing the test, another difference. The method Maybe we should start testing all methods under region util stuff to debug these errors more easily. |
@gegen07 I was working over the weekend on this and I agree 100% with you. That is something that should have been happening from the beginning, but hindsight is 20/20... This is great debugging; with my own stuff I was narrowing down on that, too. I have been working on some bash scripts that run though env creation --> solve --> plotting to highlight the differences when solving while incrementing clusters. I have found that there may even be differences outside of the (3.8 <--> 3.{9,10,11}) divide... Check the difference between Seems to be a result of untracked randomness and change in an upstream package. |
|
😵💫 |
|
@jGaboardi Gotcha! I caught the error. The mismatch over the python versions is displayed in my gist. It already has the proposed fix in Suppose my MST is something like: The example of fluctuation: This example is also a MST but is another version of it that Then these minor changes affected the random choice of clusters in the util function. The proposed fix, so, is creating the undirected graph sparse matrix based on a Maybe, there is a better fixing to it. After fixing it, I already generated the labels for AZP and all are the same now in my environment. |
|
@gegen07 great job! That sounds like a sensible solution. Can you open a PR so we can follow the implementation discussion on top of the actual code? |
|
sorry, i missed this for awhile. No, the AZP stuff is old; everything here is ported from update: oh.. if i'd actually read the whole thread, james already pointed to those |
|
@gegen07 this is amazing work! Kudos for finding this. I'm on the road today, but will be able to review later in the week if you would be able to put in a PR. Following this, let's piggy back into creating tests for each helper method/function to reduce the potential for this happening again. I am woefully behind in digging into the AZP variants we have and providing proper documentation, demonstration, and testing... |
|
Resolved via #406 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
TestAZP.test_azp_basic_from_wCI failure.diagnosis & potential causes
numpyorscipy?major versions
geopandas==0.13.2networkx== 3.1numpy==1.25.0pandas==2.0.2scipy==1.10.1shapely==2.0.1The text was updated successfully, but these errors were encountered: