Skip to content
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

3rd try at scenario building #2033

Draft
wants to merge 9 commits into
base: poc_linearProblem
Choose a base branch
from

Conversation

flomnes
Copy link
Member

@flomnes flomnes commented Apr 12, 2024

  • πŸ§ΉπŸ—‘οΈ Remove simple & legacy tests - they make it too hard to move forward with the interface
  • πŸ“œ Add basic scenarization, allowing for multiple scenarios within the same LP (πŸ’« new !)
  • πŸ“ Formatting according to src/.clang-format

TODO

  • πŸ’ͺ Adapt "standard" test-case for more than one single scenario

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to keep testing that the new fillers are compatible with the legacy problem. instead of deleting this, you can replace the new fillers it uses with the ones use in "new-workflow/standard-test"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I tried but ran into the following issue : ports are not implemented for LegacyLinearProblem/LegacyLinearProblemFillerImpl. So it's a bit hard to connect battery / thermal to legacy balance constraints. This is not related to scenarization, so I'll open another PR for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when using the ComponentFiller of "BALANCE" type, the used AddBalanceConstraint in it will hook the new balance terms to the legacy constraint (meaning you don't need a port, it is implicit). you only have to make sure the ComponentFiller has the same node name as the legacy node

Comment on lines +46 to +47
const std::map<std::string, std::vector<double>>& scalarData,
const std::map<std::string, std::vector<std::vector<double>>>& timedData) :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const std::map<std::string, std::vector<double>>& scalarData,
const std::map<std::string, std::vector<std::vector<double>>>& timedData) :
const std::map<std::string, std::vector<double>>& scalarDataPerScenario,
const std::map<std::string, std::vector<std::vector<double>>>& timedDataPerScenario) :

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be interesting to add a new test, inspired by the ones here, to test multiple scenarios

Comment on lines +80 to +83
double getObjectiveValue()
{
return objectiveValue_;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

objectiveValue_ is set at construction. I think it should be extracted after Solve. But then we have to manage a cache with synchronization issues, etc. So I suggest not caching the objective value for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MipSolution is constructed after Solve

@pet-mit pet-mit force-pushed the feature/scenario-builder branch from 39d9f3b to 08bc95c Compare April 30, 2024 08:42
@JasonMarechal25 JasonMarechal25 marked this pull request as draft July 26, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants