Kratos icon indicating copy to clipboard operation
Kratos copied to clipboard

[Core] [Properties] Remove MeshIndex from GetProperties

Open miguelmaso opened this issue 3 years ago • 10 comments

📝 Description closes #9538

Avoids a possible confusion in the python interface of ModelPart.GetProperties:

- GetProperties(prop_id, mesh_id) -> Properties instance
- GetProperties(mesh_id) -> Properties container
+ GetProperties(prop_id) -> Properties instance

🆕 Changelog

  • Remove the MeshIndex from GetProperties, it will return a Properties instance with the given Id
  • Cleanup add_model_part_to_python.cpp
  • Update python scripts from apps and core
  • Add a test

miguelmaso avatar Mar 16 '22 14:03 miguelmaso

tip: use this to find the behavior changes:

mmaso@PCCB197:~/Kratos/kratos$ grep -r "GetProperties([[:alnum:]_, ]\+)" --include "*.py"
tests/test_array_1d_interface.py:        prop = model_part.GetProperties(1,0)
tests/test_array_1d_interface.py:        prop = model_part.GetProperties(1,0)
tests/test_model_part.py:        self.assertEqual(model_part.GetProperties(0)[1].Id, 1)
python_scripts/read_materials_process.py:        model_part.GetProperties(prop_id).SetValue(CONSTITUTIVE_LAW, constitutive_law)
python_scripts/read_materials_process.py:        prop = model_part.GetProperties(property_id, mesh_id)
mmaso@PCCB197:~/Kratos/kratos$ 

miguelmaso avatar Mar 22 '22 10:03 miguelmaso

On behalf of the @KratosMultiphysics/implementation-committee, We all agree with the implementation proposed, What do you guys think? @KratosMultiphysics/technical-committee ?

AlejandroCornejo avatar Apr 20 '22 14:04 AlejandroCornejo

ping @KratosMultiphysics/technical-committee

ddiezrod avatar Jun 30 '22 07:06 ddiezrod

This is pure legacy and I fully agree with this change.

Nevertheless, I would put a visible message stating the change of behavior before changing it. Please consider that it may be used in the old context in some forks and changing the behavior would broke their code without any prompt.

pooyan-dadvand avatar Jun 30 '22 11:06 pooyan-dadvand

any advance on this?

ddiezrod avatar Aug 01 '22 08:08 ddiezrod

any advance on this?

I've just approved #9994 to start informing about this change.

In any case, I think we can safely merge this PR right away since it removes a legacy feature from the times with no submodelparts. Note that when we migrated to GitHub submodelparts were already there. Having said so, we can IMO assume that all (or the very vast majority) of our current forks are "submodelpart-based".

rubenzorrilla avatar Aug 02 '22 06:08 rubenzorrilla

any advance on this?

I've just approved #9994 to start informing about this change.

In any case, I think we can safely merge this PR right away since it removes a legacy feature from the times with no submodelparts. Note that when we migrated to GitHub submodelparts were already there. Having said so, we can IMO assume that all (or the very vast majority) of our current forks are "submodelpart-based".

Meanwhile some forks: image

loumalouomega avatar Aug 02 '22 07:08 loumalouomega

any advance on this?

I've just approved #9994 to start informing about this change.

In any case, I think we can safely merge this PR right away since it removes a legacy feature from the times with no submodelparts. Note that when we migrated to GitHub submodelparts were already there. Having said so, we can IMO assume that all (or the very vast majority) of our current forks are "submodelpart-based".

Yes, and once the index is removed we can store the mesh directly and save some precious microseconds each time we do something in the mesh

loumalouomega avatar Aug 02 '22 07:08 loumalouomega

We can merge this on 1 September so that people can see the deprecation warning

miguelmaso avatar Aug 02 '22 14:08 miguelmaso

We can merge this on 1 September so that people can see the deprecation warning

Makes sense for me. :+1:

rubenzorrilla avatar Aug 02 '22 14:08 rubenzorrilla

#10209 has reminded me that it's time to merge it

miguelmaso avatar Sep 01 '22 13:09 miguelmaso