Implement CDT bulk load method that don't panic if constraints intersect#137
Conversation
This serves as discussion starters, I imagine this comes at a performance price and will need 2 different functions.
Stoeoef
left a comment
There was a problem hiding this comment.
Thank you so much for opening this PR!
I've done a cursory review, most of this already looks pretty good.
The performance impact of using try_add_constraint should be negligible - no worries there.
However, we'll need to keep the old panic behavior for bulk_load_cdt - could you take a look into that? Thank you!
|
There are also two clippy lints. Please add an appropriate |
674d3b4 to
fb9e2f7
Compare
Reinstate the panic behaviour fixup docs
fb9e2f7 to
8785ab8
Compare
Stoeoef
left a comment
There was a problem hiding this comment.
All looks good to me. 💯
Thank you for the contribution - merging in!
I'll have a look at [try_]bulk_load_cdt_stable - maybe it can easily be improved? - and will create a new Spade version once that's done.
As #105 is labeled good first issue, I took my chances, I look forward to feedbacks to help me bring it to completion.
background
Current state
I added
try_bulk_load_cdt_stableandtry_bulk_load_cdtfunctions, as well as a test.Existing
try_bulk_load*s are affected as their functions now use thetry_add_constraints., but their behaviour is kept to panicking on conflicting edges encountered.