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] Clean up and refactor of AssignScalarFieldToEntitiesProcess and AssignScalarVariableToEntitiesProcess #12194

Merged
merged 8 commits into from
Apr 18, 2024

Conversation

loumalouomega
Copy link
Member

@loumalouomega loumalouomega commented Mar 15, 2024

📝 Description

Some clean up and refactor of AssignScalarFieldToEntitiesProcess and AssignScalarVariableToEntitiesProcess

🆕 Changelog

@loumalouomega loumalouomega added Cleanup Kratos Core FastPR This Pr is simple and / or has been already tested and the revision should be fast Refactor When code is moved or rewrote keeping the same behavior labels Mar 15, 2024
@loumalouomega loumalouomega requested a review from a team as a code owner March 15, 2024 16:28
@loumalouomega loumalouomega requested a review from roigcarlo March 15, 2024 16:28
@loumalouomega
Copy link
Member Author

Heloo @rubenzorrilla @roigcarlo

@roigcarlo
Copy link
Member

Ok, but update the doc to reflect the new historical switch

@loumalouomega
Copy link
Member Author

Ok, but update the doc to reflect the new historical switch

Those processes are not documented at all

@loumalouomega
Copy link
Member Author

@roigcarlo Doc added

@roigcarlo
Copy link
Member

@roigcarlo Doc added

Ok but next time, there was already a https://github.com/KratosMultiphysics/Kratos/blob/master/docs/pages/Kratos/Processes/Assign_Values/assign_scalar_variable_process.md which refereed to the entities generic process, just change the value. No need to duplicate, just to add the new option

@loumalouomega
Copy link
Member Author

@roigcarlo Doc added

Ok but next time, there was already a master/docs/pages/Kratos/Processes/Assign_Values/assign_scalar_variable_process.md which refereed to the entities generic process, just change the value. No need to duplicate, just to add the new option

Those are different processes, take a look

@loumalouomega loumalouomega merged commit bb1a337 into master Apr 18, 2024
13 checks passed
@loumalouomega loumalouomega deleted the core/some-processes-clean-up branch April 18, 2024 08:50
@roigcarlo
Copy link
Member

Nut sure to follow you.

The only difference is that the name of the .md is different and the fact that in the older version the entry point for master_salve is called AssignScalarVariableToConstraintsProcess and in your version is called AssignScalarVariableToMasterSlaveConstraintsProcess

@loumalouomega
Copy link
Member Author

Nut sure to follow you.

The only difference is that the name of the .md is different and the fact that in the older version the entry point for master_salve is called AssignScalarVariableToConstraintsProcess and in your version is called AssignScalarVariableToMasterSlaveConstraintsProcess

Yes, the problem is that for legacy reasons we have some processes with duplicated functionalities

@roigcarlo
Copy link
Member

roigcarlo commented Apr 18, 2024

Visible confusion ¿?

There is only one `AssignVariableToXXXXX process (for example, for nodes as they have historical values):

Screenshot_2024-04-18_11-03-05

The entry point for that process is the file AssignScalarVariableToEntitiesProcess. This process internally calls different specialized version that should not be directly used by the user which are (and that's the reason they only appear as "entry points" in the dock page I linked):

if value is number:

  • AssignScalarVariableToNodesProcess(self.model_part, params)
  • AssignScalarVariableToConditionsProcess(self.model_part, params)
  • AssignScalarVariableToElementsProcess(self.model_part, params)
  • AssignScalarVariableToMasterSlaveConstraintsProcess(self.model_part, params)

if value is vector

  • AssignScalarFieldToNodesProcess(self.model_part, params)
  • AssignScalarFieldToConditionsProcess(self.model_part, params)
  • AssignScalarFieldToElementsProcess(self.model_part, params)

There is no duplicity here, the only think that has changed is that now there is an additional json config that allows the AssignScalarVariableToEntitiesProcess to also be able to call

  • AssignScalarVariableHistoricalToNodesProcess(self.model_part, params)
  • AssignScalarFieldHistoricalToNodesProcess(self.model_part, params)

if the new switch you added is on.

Those new options and entry points is what I meant you to add in the doc.

@matekelemen
Copy link
Contributor

@loumalouomega why did you adapt this process for MasterSlaveConstraints? Are you using the DataValueContainer of MasterSlaveConstraints somewhere?

I'm asking because neither MasterSlaveConstraint nor LinearMasterSlaveConstraint actually uses the DataValueContainer member MasterSlaveConstraint::mData, and I'm thinking about removing it because it's confusing.

@matekelemen
Copy link
Contributor

Btw it seems to me that you could have used proxies for this PR.

@loumalouomega
Copy link
Member Author

Btw it seems to me that you could have used proxies for this PR.

I didn't knwo about that

@loumalouomega
Copy link
Member Author

@loumalouomega why did you adapt this process for MasterSlaveConstraints? Are you using the DataValueContainer of MasterSlaveConstraints somewhere?

I'm asking because neither MasterSlaveConstraint nor LinearMasterSlaveConstraint actually uses the DataValueContainer member MasterSlaveConstraint::mData, and I'm thinking about removing it because it's confusing.

To be consistent...

@loumalouomega
Copy link
Member Author

@loumalouomega why did you adapt this process for MasterSlaveConstraints? Are you using the DataValueContainer of MasterSlaveConstraints somewhere?
I'm asking because neither MasterSlaveConstraint nor LinearMasterSlaveConstraint actually uses the DataValueContainer member MasterSlaveConstraint::mData, and I'm thinking about removing it because it's confusing.

To be consistent...

I am using it and it is used somewhere...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup FastPR This Pr is simple and / or has been already tested and the revision should be fast Kratos Core Refactor When code is moved or rewrote keeping the same behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants