Repository navigation
Conversation
…est introduction. This contains the addition to Join functionality in string_utilities.cpp too. Unit tests for process_utilities and string_utilities have been adapted and extended.
avdg81
left a comment
avdg81
left a comment
There was a problem hiding this comment.
Dear Wijtze Pieter,
Thank you very much for moving these changes to a separate PR, and for addressing my previous suggestions. I have three more very minor suggestions (which I had either forgotten to mention before or which were raised by SonarQube). I think this PR is almost ready to go. 👍
- use of std::ranges - transparent std::less<> for string comparison - string literals used in header file.
avdg81
left a comment
avdg81
left a comment
There was a problem hiding this comment.
Hi Wijtze Pieter,
Thanks for addressing the final suggestions that I had. From my side, this PR looks good to go. Let's wait for @markelov208's review of this PR. Thanks.
markelov208
left a comment
markelov208
left a comment
There was a problem hiding this comment.
Hi Wijtze Pieter, many thanks for this extention for processes that were never used before. I have a few coments. If you have any question, please let me know.
| std::string_view Prefix) | ||
| { | ||
| const std::set<std::string, std::less<>> processes_having_computing_model_part_name = { | ||
| "AssignAverageMasterSlaveConstraintsProcess"s, "ApplyPeriodicConditionProcess"s, "SkinDetectionProcess"s}; |
There was a problem hiding this comment.
Copilot AI: Optional computing-part names can break valid process settings. In process_utilities.cpp:55-61, the three recognized processes are required to provide computing_model_part_name in the raw project settings. But apply_periodic_condition_process.py:30-40 and skin_detection_process.py:14-28 supply defaults for that setting, and the GeoMechanics solver scans settings before constructing processes (geomechanics_solvers_wrapper.py:36-40). Omitting the key therefore throws before those defaults apply. For periodic conditions, explicitly using the valid empty default also adds "" to the generated list; preparation then attempts to resolve it as a submodelpart.
Shall only AssignAverageMasterSlaveConstraintsProcess be added now?
There was a problem hiding this comment.
Generally I think that a process parameters file should contain the process_name records.
The copilot remark about GeoMechanics solver scans settings before constructing processes generally holds, otherwise we could not create process_sub_model_part_list the way we do now.
I would like to keep all 3 process names in the given list, otherwise the knowledge that they need computing_model_part_name is lost, whilst any user can still add them to a computation with GeoMechanicsApplication.
| if (!r_process.Has("Parameters")) continue; | ||
| const auto model_part_names = GetProcessModelPartNames(r_process["Parameters"], {}); | ||
| const auto model_part_name_keys = | ||
| r_process.Has("process_name") && |
There was a problem hiding this comment.
As far as I understand r_process.Has("process_name") is to prevent a problem with the next line. By now none of our integration tests use process with missing process_name. Only a few unit tests do not use it. Shall process_name be mandatory for all processes?
Secondly, it seems these new lines are not tested with unit test, right?
There was a problem hiding this comment.
The r_process.Has("process_name") is to let all behavior that we not have discovered yet use the current and previous default of looking at model_part_name and model_part_name list.
Indeed coverage analysis on these lines did not show use yet, so I added a test using only of the master slave processes that needs computing_model_part_name.
There was a problem hiding this comment.
Tried to find out if always a process should have process_name.
That does not seem to be the case. Part of the AI answer:
When process_name Is Optional or Omitted
• Custom / Python-defined processes: If a process is instantiated directly in your Python script (MainKratos.py) or uses a custom wrapper rather than Kratos's automatic process factory, the C++ or Python constructor may only look for python_module, kratos_module, and the inner Parameters dictionary.
There was a problem hiding this comment.
Here is what AI found:
python_module tells Kratos which Python module to import. For the usual module-based process path, the core factory calls that module’s Factory(item, Model); it does not require process_name itself (core process factory, process_factory.py:68).
A few processes use process_name. Core process_factory: ApplyConstantScalarValueProcess, ApplyConstantVectorValueProcess, TimeAveragingProcess. Its Factory reads process_name to choose the C++ process.
AssignAverageMasterSlaveConstraintsProcess, ApplyPeriodicConditionProcess, SkinDetectionProcess do not require process_name.
What is your opinion: is it better to use python_module instead of process_name?
rClampCharacter -> rNameWrapper
tested extra condition in process_utilities
This branch has not been deployed
4 participants
📝 Description
Split off the computing_model_part_name acceptance from the biaxial test introduction.
This contains the addition to Join functionality in string_utilities.cpp too.
Unit tests for process_utilities and string_utilities have been adapted and extended.