Generalzie pyomo model handling#55
Conversation
fletchapin
left a comment
fletchapin
left a comment
There was a problem hiding this comment.
Overall looks good! I left a few minor comments, and I can just fix them myself if that's easiest. Then we should be ready to merge
fletchapin
left a comment
fletchapin
left a comment
There was a problem hiding this comment.
Thanks for all the changes! There is just one bug remaining that I noticed. With the new type checking, our assumption that IndexedExpression was a subclass of Expression was incorrect, so we'll have to update that logic accordingly.
| if (not check_nonindexed_python_type(expression2)) and ( | ||
| len(expression2) > 1 | ||
| ): | ||
| # TODO: replace model.t with better way to get dimensions |
There was a problem hiding this comment.
We can remove this comment, since we are completing the to-do :)
| ) | ||
|
|
||
|
|
||
| def check_indexed_python_type(input_var): |
There was a problem hiding this comment.
If we are going to have this single indexed type might as well add a "dataframe like" checker that checks Pandas, Polars, etc.
There was a problem hiding this comment.
To be pedantic, I would say that lists are an indexed Python type (that we treat differently), so I might rename the function
There was a problem hiding this comment.
I guess we only support numpy arrays anyway, so I changed it to check_indexed_np_array.
I think if we need a df/polars check it would be a different function entirely as places where we accept np_array, we do not accept df/polars (e.g. I think if you get np.array there it will fail...
| for key, var in consumption_data_dict.items(): | ||
| if isinstance(var, dict): | ||
| for svar in var.values(): | ||
| if isinstance(svar, (cp.Expression, pyo.Var, pyo.Param)): |
There was a problem hiding this comment.
Sorry if it's obvious and I missed it, but why is this check different than the one on L1662? (https://github.com/we3lab/eeco/pull/55/changes#diff-0122b06abba34b1b9a0a5d73a537d39f7fad05573275d50358d5b19fc8f60d35R1662)
There was a problem hiding this comment.
Uhh, i missed this some how...
…elying on old model.t
3 participants
Fix issue with pyomo model handling.
Right now its assumed pyomo model has a variable t on it which is used for indexing the consumption variable, this is not necessarly true and complicates model handling.
Here we update the costing to automatically extract index set from the consumption variable, and use it through out. This removes the need to find a model.t var or for user to create it.
This additionally fixes tests: