Rewrite core.relax_integer_vars transformation#3586
Conversation
…o into modernize-relax-integer-vars
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
Accidentally hit merge when I meant to update the branch from main. This is fine, with three small comments (none of which would have prevented merging).
| for b in block.component_data_objects(Block, active=None, descend_into=True): | ||
| if not b.active: | ||
| if config.transform_deactivated_blocks: | ||
| deprecation_warning( | ||
| "The `transform_deactivated_blocks` arguments is deprecated. " | ||
| "Either specify deactivated Blocks as targets to activate them " | ||
| "if transforming them is the desired behavior.", | ||
| version='6.9.3.dev0', | ||
| ) | ||
| else: | ||
| continue |
There was a problem hiding this comment.
Some questions:
- Why descend only over
Blockwhen you acceptBlockandDisjunctas targets? Should this becomponent_data_objects(SubclassOf(Block), ...? - Why test for
activeinside the loop? Wouldn't it be easier toactive = None if config.transform_deactivated_blocks else True? Is the idea to only issue the deprecation warning if model actually had a deactivated block?
There was a problem hiding this comment.
Yes, all of this is to make sure we only issue the deprecation warning if the model actually has a deactivated block that we're transforming. When we remove the argument, all this complexity can go away and we can just do what you suggest above. I just didn't want to be loud in all the cases where this is fine because (annoyingly) the default value for transform_deactivated_blocks is True. So without this mess I would just be raising deprecation warnings when the user didn't actually do anything--they probably don't know the option exists.
4 participants
Fixes # .
Summary/Motivation:
This rewrites the
core.relax_integer_varstransformation, modernizing it to our more recent transformation conventions. In particular, it:undoargument in favor of implementingreverseaccording to the design we introduced in Adding (reversible)gdp.transform_current_disjunctive_logictransformation #2809.targetsargumentcomponent_data_objects(Var)Changes proposed in this PR:
core.relax_integer_varstransformationVarCollectorenum to distinguish getting Vars from active expressions vs. usingcomponent_data_objects(Var). Naming suggestions are welcome here...Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: