Disallow calling del_component with ComponentData arguments#3440
Disallow calling del_component with ComponentData arguments#3440blnicho merged 15 commits intoPyomo:mainfrom
del_component with ComponentData arguments#3440Conversation
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
I am pretty sure that the change to del_component won't actually catch attempts to delete a component data.
pyomo/core/base/block.py
Outdated
| if isinstance(obj, ComponentData) and not isinstance(obj, Component): | ||
| raise ValueError( | ||
| "Argument '%s' to del_component is a ComponentData object. Please " | ||
| "use the Python 'del' function to delete members of indexed " | ||
| "Pyomo objects. The del_component function can only be used to " | ||
| "delete IndexedComponents and ScalarComponents." % name | ||
| ) | ||
|
|
There was a problem hiding this comment.
This doesn't do what you think it does: line 1133 (obj = self.component(name_or_object)) maps the incoming object into its component, so you shouldn't be able to trigger this exception. If we are going to change the behavior of del_component to only admit Components (either by name or reference), we should
- change (possibly inline the logic from) line 1133
- document this in the docstring
- possibly add a deprecation path (because we are changing behavior that has been in Pyomo for over a decade)??
There was a problem hiding this comment.
I would argue that this is a bug and does not need a deprecation path.
There was a problem hiding this comment.
I think block.component() only works with strings if the string refers to a component and not a component data, so we can probably just check if name_or_object is a string?
…thing that doesn't exist
|
I agree with @michaelbynum that this is a bug and shouldn't have a deprecation path. Especially after I saw where it got hit in the codebase already... Assuming tests pass (I think they finally will?), this is ready for review again. |
|
We are probably not going to aim to merge this before the patch release next week (because it's fairly intricate / could break some folks). We don't want to sneak this in without sufficient planning/warning. |
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
One minor question, but overall this looks good.
|
This is ready to merge when tests pass. |
github-project-automation
bot
moved this from Reviewer Approved
to Done
in August 2025 Release
6 participants
Fixes #3435
Summary/Motivation:
I think silently deleting the containing component when we call
del_componenton a ComponentData is rude, and the post-processing in Fourier-Motzkin elimination was falling victim to this mistake. This PR adds aValueErrortodel_componentif it is given a ComponentData and revises the FME transformation post-processing routine.Changes proposed in this PR:
del_componentif the arguments is a ComponentDatadel_componentand, in fact, to not add and delete as many components in general.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: