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

full fua testing for simplify_network() #41

Merged
merged 20 commits into from
Oct 16, 2024
Merged

Conversation

jGaboardi
Copy link
Collaborator

@jGaboardi jGaboardi commented Oct 14, 2024

@jGaboardi jGaboardi self-assigned this Oct 14, 2024
@jGaboardi
Copy link
Collaborator Author

Failures we are seeing are unfortunately not due to small differences in testing precision, but actual _status difference. For example, in Aleppo:

>       assert_series_equal(known._status, observed._status)

sgeop/tests/test_simplify.py:44: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
testing.pyx:55: in pandas._libs.testing.assert_almost_equal
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   AssertionError: Series.index are different
E   
E   Series.index values are different (0.02026 %)
E   [left]:  Index([    0.0,     1.0,     2.0,     3.0,     4.0,     5.0,     6.0,     7.0,
E              8.0,     9.0,
E          ...
E          39658.0, 39659.0, 39660.0, 39666.0, 39668.0, 39669.0, 39670.0, 39676.0,
E          39678.0, 39679.0],
E         dtype='float64', length=39489)
E   [right]: Index([    0.0,     1.0,     2.0,     3.0,     4.0,     5.0,     6.0,     7.0,
E              8.0,     9.0,
E          ...
E          39658.0, 39659.0, 39660.0, 39666.0, 39668.0, 39669.0, 39670.0, 39676.0,
E          39678.0, 39679.0],
E         dtype='float64', length=39489)
E   At positional index 2488, first diff: 2496.0 != 2494.0

testing.pyx:173: AssertionError

I think there is still a good chance that cross-version and -platform installs are contributing to this within sgeop, but it will be much more difficult to solve than simply bumping up the tolerance in testing...

@martinfleis
Copy link
Contributor

There seems to be some difference in how something gets computed on Apple Silicon.

@jGaboardi
Copy link
Collaborator Author

Yeah Apple Silicon sticks out, but there seem to be differences across all CI environments.

@jGaboardi
Copy link
Collaborator Author

@martinfleis after some blood, sweat, & tears I've set up our action to curate the observed simplified network data as artifacts so at least we can easily compare between OS & package versions for results.

@martinfleis
Copy link
Contributor

Good job! I think that there won't be a ton of what we could do here as this is most likely due to some precision differences in underlying C code (in Qhull, GEOS...) But good to have the ability to debug.

@martinfleis
Copy link
Contributor

I looked into the differences. Vast majority is precision thing. But there are some visible differences here and there. Not sure how much they matter though and if there's any chance we could prevent those.

My proposal is to check the equality of simplified stuff only on one OS in CI.

@jGaboardi
Copy link
Collaborator Author

My proposal is to check the equality of simplified stuff only on one OS in CI.

Sounds good. I am thinking latest & Apple Silicon since that's what I have, but I am open to another version/OS combo.

As for non-equality testing of the full FUA simplification, is there anything else we should/could check for?

@jGaboardi
Copy link
Collaborator Author

note to self: open another issue to include some documentation about this:

... due to some precision differences in underlying C code (in Qhull, GEOS...)
... is precision thing. But there are some visible differences here and there.

@martinfleis
Copy link
Contributor

Sounds good. I am thinking latest & Apple Silicon since that's what I have, but I am open to another version/OS combo.

I'd probably do ubuntu as that is what anyone can get in a container. Apple Silicon is a bit exclusionary.

@jGaboardi
Copy link
Collaborator Author

As for non-equality testing of the full FUA simplification, is there anything else we should/could check for?

@martinfleis Do you have anything opinions/ideas for this? I suppose we a try testing .shape but even that might be different. I will try that here in another commit.

@martinfleis
Copy link
Contributor

Even shape is different in some cases... I don't know what to check. Rough sum of distances?

@jGaboardi
Copy link
Collaborator Author

hmmmm. Let's think more on this.

@jGaboardi
Copy link
Collaborator Author

Rough sum of distances?

I'll push this up in a commit and swap out the current "known" simplified for that produced by Ubuntu latest. See where that gets us.

@jGaboardi
Copy link
Collaborator Author

We've got 5/7 passing envs by testing against the known results from Ubuntu Python 3.12 latest

@jGaboardi
Copy link
Collaborator Author

I want to get the conftest.py started in #27 merged, then build the logic here to only do equality tests from certain CI environments.

@jGaboardi jGaboardi merged commit 5c033a4 into main Oct 16, 2024
8 checks passed
@jGaboardi jGaboardi deleted the GH38_full_fua_testing branch October 16, 2024 13:45
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.

full FUA(s) for full algo run
2 participants