Skip to content

Disallow calling del_component with ComponentData arguments #3440

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

Merged
merged 15 commits into from
Jun 28, 2025

Conversation

emma58
Copy link
Contributor

@emma58 emma58 commented Dec 2, 2024

Fixes #3435

Summary/Motivation:

I think silently deleting the containing component when we call del_component on a ComponentData is rude, and the post-processing in Fourier-Motzkin elimination was falling victim to this mistake. This PR adds a ValueError to del_component if it is given a ComponentData and revises the FME transformation post-processing routine.

Changes proposed in this PR:

  • Sneaks in a documentation fix for ComponentData
  • Adds error to del_component if the arguments is a ComponentData
  • Revises FME post-processing routine to not misuse del_component and, 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:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure that the change to del_component won't actually catch attempts to delete a component data.

Comment on lines 1143 to 1150
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
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)??

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that this is a bug and does not need a deprecation path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@emma58
Copy link
Contributor Author

emma58 commented Mar 25, 2025

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.

@mrmundt
Copy link
Contributor

mrmundt commented Apr 8, 2025

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.

Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor question, but overall this looks good.

@blnicho blnicho moved this from Todo to Review In Progress in July 2025 Release Jun 23, 2025
@emma58
Copy link
Contributor Author

emma58 commented Jun 24, 2025

This is ready to merge when tests pass.

@blnicho blnicho moved this from Review In Progress to Reviewer Approved in July 2025 Release Jun 24, 2025
@blnicho blnicho merged commit 874352c into Pyomo:main Jun 28, 2025
35 checks passed
@github-project-automation github-project-automation bot moved this from Reviewer Approved to Done in July 2025 Release Jun 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Calling del_component on a ComponentData deletes the parent component
6 participants