-
Notifications
You must be signed in to change notification settings - Fork 30
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
Change Model::setStateIsNonNegative logic #1648
Conversation
Previously `model.setStateIsNonNegative(model.getStateIsNonNegative())` failed if conservation laws were enabled. That feels unintuitive. Now it works.
Codecov Report
@@ Coverage Diff @@
## develop #1648 +/- ##
===========================================
+ Coverage 78.81% 78.87% +0.05%
===========================================
Files 69 69
Lines 10717 10717
===========================================
+ Hits 8447 8453 +6
+ Misses 2270 2264 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
src/model.cpp
Outdated
@@ -609,7 +609,11 @@ std::vector<bool> const &Model::getStateIsNonNegative() const { | |||
} | |||
|
|||
void Model::setStateIsNonNegative(std::vector<bool> const &nonNegative) { | |||
if (nx_solver != nx_rdata) { | |||
auto any_state_non_negative = std::any_of(state_is_non_negative_.begin(), |
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.
I am not sure what's implemented here, shouldn't this be applied to nonNegative
? Not implementing this was primarily a result of lazyness. If we specify what kind of indices are passed here (solver or rdata x), we could also support setting non_negative states, just need to map between the two.
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.
True. Thanks.
I'll open a separate issue for that. #1649
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.
If we specify what kind of indices are passed here (solver or rdata x), we could also support setting non_negative states, just need to map between the two.
With x_solver indices we wouldn't have to check whether it actually is a solver state. However, I'd guess that most users wouldn't want to care about x_solver at all, and therefore, x_rdata would be more appropriate. Or we just do it ID-based. That's probably the most convenient. What would be your preference?
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.
If we specify what kind of indices are passed here (solver or rdata x), we could also support setting non_negative states, just need to map between the two.
With x_solver indices we wouldn't have to check whether it actually is a solver state. However, I'd guess that most users wouldn't want to care about x_solver at all, and therefore, x_rdata would be more appropriate. Or we just do it ID-based. That's probably the most convenient. What would be your preference?
I agree, I would expect that users want to pass x_rdata. For id based specification we should probably add another function as changing this function would be API breaking.
Kudos, SonarCloud Quality Gate passed! |
Previously
model.setStateIsNonNegative(model.getStateIsNonNegative())
failed if conservation laws were enabled. That feels unintuitive.
Now it works.