-
Notifications
You must be signed in to change notification settings - Fork 35
Make MST network test deterministic #1046
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
Conversation
|
🚨 API breaking changes detected! 🚨 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1046 +/- ##
==========================================
- Coverage 94.54% 92.79% -1.75%
==========================================
Files 134 134
Lines 9982 9984 +2
==========================================
- Hits 9437 9265 -172
- Misses 545 719 +174
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
IAlibay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
IAlibay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually backing out a litlte bit - isn't this also likely to have clashes because they all have the same score? I.e. it's still sort dependent?
|
There should only be a single possible MST with these new edge scores regardless of the graph's ordering though where as the current failures are due to multiple possible MST networks and the returned one is selected based on the ordering of the edges. We also seem to get the same network on main and on the last working commit on gufe so hopefully that shows its not order dependent. |
IAlibay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation - lgtm.
Fixes #1037 by making the edge scores deterministic, this should ensure we always get the same networks regardless of edge ordering.
Checklist
newsentryDevelopers certificate of origin