-
Notifications
You must be signed in to change notification settings - Fork 94
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
Feature: piecewise constraints #569
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #569 +/- ##
==========================================
+ Coverage 95.92% 95.97% +0.05%
==========================================
Files 26 26
Lines 3899 3980 +81
Branches 838 767 -71
==========================================
+ Hits 3740 3820 +80
- Misses 69 70 +1
Partials 90 90
|
docs/user_defined_math/examples/sos2_piecewise_linear_costs.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Stefan Pfenninger <[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.
At a high level it looks good to me. If @irm-codebase has capacity for a more detailed technical review that would be great.
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.
@brynpickering review done!
Overall, it looks good. But I do have some questions regarding new additions to the backend, and potentially removing some lines that do not seem to do anything.
As always, nice-to-haves are just that. Feel free to ignore 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.
Nice-to-have: at the moment, it's a tad hard to find this if you do not know about this functionality.
This relates to a separate issue (#601), but we should consider improving this in the future, maybe by listing these features in the Home directory.
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.
Yeah, as mentioned in another comment, having a key features page is high up on our to-do list. @sjpfenninger is on the case!
new_params = { | ||
"parameters": { | ||
"capacity_steps": { | ||
"data": capacity_steps, |
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.
Nice-to-have:
For ease of reading, I would either:
- re-define
capacity_steps
andcost_steps
abovenew_params
, so users see them when reading this bit of the example. - just pasting the array (e.g.,
[0, 2500, 5000, 7500, 10000]
) atdata
.
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.
They are defined in the previous Python cell (with only one markdown cell in between). I'd say that's close enough to be visible. Still, I'll print out new_params
after defining it.
# In this example, we require a new decision variable for investment costs that can take on the value defined by the curve at a given value of `flow_cap`. | ||
|
||
# %% | ||
m.math["variables"]["piecewise_cost_investment"] = { |
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.
Nice-to-have:
Consider showing this added math in YAML
format. Particularly because the example in Math components is different, and we may not want to incentivise users messing with model.math
directly.
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.
hmm, this perhaps exposes a problem with adding math snippets on-the-fly. I'd need to show it in YAML and then also store that in a file AND reference that in config.init.add_math
. Would your math fixes make that easier?
@@ -977,5 +977,7 @@ Now, all components of our internal math are defined in a readable YAML syntax t | |||
|
|||
You can add your own math to update the pre-defined math and to represent the physical system in ways we do not cover in our base math, or to apply new modelling methods and problem types (e.g., pathway or stochastic optimisation)! | |||
|
|||
When adding your own math, you can add [piecewise linear constraints](user_defined_math/components.md#piecewise-constraints), which is a new type of constraint compared to what could be defined in v0.6. |
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.
Nice-to-have: not sure if this belongs here...
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.
Yeah, we need a quick intro to everything you can do in Calliope v0.7, which we don't have at the moment. Hence why some new functionality is described in migrating.md
, as it's where existing users are most likely to go first. I'll leave it in until we have this intro page that @sjpfenninger might be putting together.
component_dict = self.inputs.math[component_type][name] | ||
if name not in self.inputs.math[component_type]: | ||
self.inputs.math[component_type][name] = component_dict | ||
if name not in self.inputs.math.get(component_type, {}): |
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 might be possible to remove this check...
Looking at the order of calls, it seems like this case can never be hit.
name
appears to always come from self.inputs.math[components].items()
, in self.add_all_math
.
Since this is an internal function, removal might be ok. Let me know what you think.
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.
no, you can simply do model.backend.add_constraint(...)
and it will add a new constraint to the backend after you've added everything from the math dict. This then appends it to the math dict.
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.
Hmmm... this suggests that we are not arranging logic in the best way. If that condition can only be reached when calling add_x
, then this should be handled at that level.
Maybe setting a new math
key should happen in add_x
(keeping the override off, to catch duplicates), and this function should just add the component to the backend?
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.
Except then you have to handle in every add_x
method separately, including across backends (something this add_component
helper func was made to deal with). It could be added as a wrapper on all add_x
funcs, I guess?
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.
Hmmm... I do not think that is the case, because component_type
and component_dict
are passed by the caller, but repeating it everywhere seems off, I agree. He probably have an antipattern.
Let's keep it for now, as this is another issue.
else: | ||
yaml_snippet_attrs[attr] = val | ||
|
||
if yaml_snippet_attrs: |
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.
Is there a particular reason why we check/have this in the Model generator and not in the Latex backend?
Seems like this is only ever used on that one.
Might be a case of a subclass polluting parent functionality?
if evaluated.shape: | ||
dims = rf"_\text{{{','.join(str(i).removesuffix('s') for i in evaluated.dims)}}}" | ||
if evaluated.attrs["obj_type"] != "string": | ||
data_var_string = evaluated.attrs["math_repr"] |
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.
Similar to the yaml attribute above: this seems like it's only used by the Latex
backend, but it's present in the regular backend. Not necessarily something to fix now, but it hints that child classes may be polluting the parent.
Not something to fix in this PR, but is this desired?
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 math representation? I guess it is only of use to the LaTeX backend. Definitely something to clean up at some point
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.
Correct!
I do not think it's super bad, these attributes do not use much memory. But it suggests that our logic is getting murky.
Fixes #107
Summary of changes in this pull request:
piecewise_constraints
component to YAML math.TODO
- [ ] Test values as lists and not as mutable parameters.EDIT: not addingReviewer checklist: