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

Inconsistent ModelPart.GetProperties #9538

Closed
miguelmaso opened this issue Jan 14, 2022 · 4 comments · Fixed by #9774
Closed

Inconsistent ModelPart.GetProperties #9538

miguelmaso opened this issue Jan 14, 2022 · 4 comments · Fixed by #9774

Comments

@miguelmaso
Copy link
Contributor

Description

I've observed the following discrepancy in the ModelPart interface:

  • From the c++ interface, ModelPart.GetProperties(id) returns the property with the corresponding id from the mesh 0 (default value)
  • From the python interface, ModelPart.GetProperties(id) returns all the properties from the mesh with the corresponding id.
@loumalouomega
Copy link
Member

I opened a discussion about this in the past, and I don't remember why we keep it (I think because it would break to much code)

@miguelmaso
Copy link
Contributor Author

miguelmaso commented Mar 16, 2022

Recap

item container
Value Node Nodes
Element Elements
Condition Conditions
Properties Properties
Methods CreateNew(item)
Has(item)
Get(item)
Add(item)
Remove(item)
(container)
Set(container)
  • Properties means the item and also the container.
  • The Python interface adds Get(container). In general, this is not a problem, but it conflicts in the case of the properties. Consequently, there are multiple definitions of GetProperties in Python, returning the container or the item.
  • Furthermore, the properties Id may be confused with the mesh Id (legacy).

My proposal

  1. Remove the mesh Id from the python interface ([Core] [Properties] Remove MeshIndex from GetProperties #9774). This is a behavior change.
- GetProperties(mesh_id) -> Container
+ GetProperties(prop_id) -> Item
  1. Remove the GetProperties() (container), which can be confused with GetProperties(id) (item). This is an API breaker.

@miguelmaso
Copy link
Contributor Author

miguelmaso commented Mar 16, 2022

NOTE: I'm not sure about the proposal 2 and I would like to hear opinions.

Pros:

  • Avoid confusion between properties item and properties container
  • Easy to replace

Cons:

  • There is a GetNodes etc, but not GetProperties (container)
  • ~1k of deprecation warnings

@miguelmaso
Copy link
Contributor Author

miguelmaso commented Mar 22, 2022

In @KratosMultiphysics/implementation-committee we prefer only to remove the mesh id (option 1)

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

Successfully merging a pull request may close this issue.

2 participants