-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add specific dispatch for >=
constraints.
#81
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #81 +/- ##
==========================================
+ Coverage 80.92% 84.76% +3.83%
==========================================
Files 31 30 -1
Lines 1143 1385 +242
==========================================
+ Hits 925 1174 +249
+ Misses 218 211 -7 ☔ View full report in Codecov by Sentry. |
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.
Just leaving behind some comments, not to block the merge. Feel free to address them
s::GT{T}, | ||
arch::AbstractArchitecture, | ||
) where {T} | ||
# Scalar Affine Inequality Constraint: g(x) = a'x - b ≥ 0 |
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.
Usually the notation always covers the <= case first. Why this choice?
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.
The idea is that we have internal functions who will be shared by both cases. We want to avoid using bridges for this and handle them manually instead.
|
||
return nothing | ||
end | ||
|
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.
There are several warnings from codecov where lines are not covered by tests. Should we work on these before merging?
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.
Most are due to new attributes, it will be straightforward to cover them.
end | ||
|
||
return nothing | ||
end |
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.
It is a good test, but I like that tests also highlight examples from the literature so that we can always reference them. Can we use some of the ones that I showed in recent documents?
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.
Of course!
This PR is a first move towards avoiding bridges, as part of an initiative to better interact with JuMP's attribute system.