Skip to content
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

[Core] Forbid Renaming ModelParts #12148

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matekelemen
Copy link
Contributor

Description

ModelPart allows resetting its name, which breaks sub model part containers. These containers are hash maps that generate their hashes based on the model part's name. However, when a ModelPart is renamed, the sub model part container of its parent doesn't update its hash map, which completely breaks access to sub model parts.

Changelog

  • remove std::string& ModelPart::Name() (the mutable overload)
  • remove Model::RenameModelPart and its corresponding test

@loumalouomega
Copy link
Member

What about update the hash map?

Copy link
Member

@loumalouomega loumalouomega left a comment

Choose a reason for hiding this comment

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

Requires further discussion

@loumalouomega
Copy link
Member

What about update the hash map?

We can make the Name() method const and update the implementation of the Rename method

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

makes full sense to me, I think this is a leftover from early Model days

Comment on lines -1811 to -1815
std::string& Name()
{
return mName;
}

Copy link
Member

Choose a reason for hiding this comment

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

yikes, time to go 👋

Copy link
Member

Choose a reason for hiding this comment

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

In this particualr case yes, it makes sense

Copy link
Member

Choose a reason for hiding this comment

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

add_model_part_to_python.cpp:46:25: error: no match for 'operator=' (operand types are 'const std::string' {aka 'const std::__cxx11::basic_string<char>'} and 'const std::string' {aka 'const std::__cxx11::basic_string<char>'})

Looks like python binding is a bit hardcoded

Copy link
Member

Choose a reason for hiding this comment

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

Algo agree with this one

@matekelemen
Copy link
Contributor Author

What about update the hash map?

We can make the Name() method const and update the implementation of the Rename method

Is there a valid use case tho? I'd rather just remove the entire thing than move ModelParts around.

@loumalouomega
Copy link
Member

What about update the hash map?

We can make the Name() method const and update the implementation of the Rename method

Is there a valid use case tho? I'd rather just remove the entire thing than move ModelParts around.

Well, i guess if the method was there is beacuse somone was using it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants