-
Notifications
You must be signed in to change notification settings - Fork 1
17 skip mpc #26
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
base: main
Are you sure you want to change the base?
17 skip mpc #26
Conversation
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 like the feature, but I think it should not be in basicmpc.
There is a file mpc_full, which can be used to add more complicated features to mpc.
In this case, I think I would prefer a general approach, where the mpc has an input that activates or deactivates it, which can then be sent from a more general module.
Another possibility, if you would like to keep this feature unchanged, is creating a subclass, i.e. time-conditional mpc or something like that. I just want to make sure that the basempc remains readable for people that want to learn how to handle an mpc module.
@EserSteffen : I agree and added the most general approach by introducing an AgentVariable in the base mpc. Do you agree with the format? The module to skip the MPC does not need to be in the modules, but I wanted to add an example to test the function. |
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md # agentlib_mpc/__init__.py
Edited it myself, now need external review
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.
for me its fine now, please check everything works as expected after my change
@EserSteffen : The example works now and the CI passes, so you could check again and approve? |
This reverts commit dea0e19
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.
Only a minor comment. Rest LGTM.
if results and "myMPCAgent" in results and "AgentLogger" in results["myMPCAgent"]: | ||
agent_logger = results["myMPCAgent"]["AgentLogger"] | ||
# Try "mpc_active" first, then "MPC_FLAG_ACTIVE" | ||
for var_name in ["MPC_FLAG_ACTIVE"]: |
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.
Use datamodels var?
@EserSteffen the feature worked really well in a local test, I will now do annual simulations. Is the naming and style of the change fine for you?