-
Notifications
You must be signed in to change notification settings - Fork 21
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
Update TulipaConstraint to allow attaching more constraints to the same indices table #959
Update TulipaConstraint to allow attaching more constraints to the same indices table #959
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #959 +/- ##
==========================================
- Coverage 95.73% 95.42% -0.32%
==========================================
Files 29 29
Lines 1008 1027 +19
==========================================
+ Hits 965 980 +15
- Misses 43 47 +4 ☔ View full report in Codecov by Sentry. |
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
@abelsiqueira option 1 is preferred, and I see you started with it. So, Thanks! |
5c0730e
to
2b0fdbc
Compare
Thanks for the review @datejada, I commented on all of them |
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.
Hi @abelsiqueira, thanks for the PR. There were some comments and questions during the review, but generally speaking, it looks cleaner with the changes, and I like the approach. Thanks.
Thanks for the review! I've updated the PR with the error messages and further comments. Let me know what you think |
Co-authored-by: Diego Alejandro Tejada Arango <[email protected]>
Co-authored-by: Diego Alejandro Tejada Arango <[email protected]>
Co-authored-by: Diego Alejandro Tejada Arango <[email protected]>
Co-authored-by: Diego Alejandro Tejada Arango <[email protected]>
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.
Great! Thanks for the changes; I am approving this PR. You can merge after all the checks have been done.
Thanks! I'll merge after the benchmarks run and rebase #642 |
With the idea of allowing multiple constraints per indices table, this links the indices to the constraints. It also creates a function
attach_constraint!
to help keep things in check.It fails the avoid fields with abstract types recommendation (
ConstraintRef
has three parametric types), so it might be unnecessarily slow.I see two solutions:
EDIT: I added a commit with option 1 implemented.
@datejada, what do you think?
Closes #957