[GeoMechanicsApplication] Phreatic multi line #11184
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added tests Fixed issue
Destructors are special member functions that are overridden when the base class destructor is `virtual`. Don't supply empty function bodies, but use `= default` instead. Note that this is in line with C.80: "Use `=default` if you have to be explicit about using the default semantics" from the C++ Core Guidelines.
Some macro usages were followed by a semicolon, whereas this was not needed. As a result, we had some empty statements. Those have been removed now.
To comply with Kratos' Style Guide several variables names were converted from lowerCamelCase to snake_case.
The implementations of these overrides were identical to the implementations of their base class counterparts. In fact, all function bodies were empty. Therefore, there was no point in overriding the base class behaviour, since they were identical. Removed the overrides.
The fixes include: - Removed a few unused variables from a Python script. - Added two early exits to reduce the number of nested `if` statements. - Avoid narrowing due to implicit converions. Use `static_cast`s to make clear that the conversions are intentional. - Removed a variable from the C++ code that did not have any added value. We now use the returned value directly rather than assigning it first. - In line with C.81 "Use `=delete` when you want to disable default behavior (without wanting an alternative)" of the C++ Core Guidelines, two private copy assignment operators and two private copy constructors have been deleted. - Replaced one `typedef` by `using`. - Avoid hiding an inherited non-virtual function. - Replaced a raw `for` loop by a standard transformation.
- Attempted to reduce the Cognitive Complexity of a Python function. * Reduced the maximum number of nested `if` statements. * Reordered some code to make it read more naturally. * Renamed a few variables. - Converted several protected data members to private data members. For most of them getters have been added, too. In this way, we comply with C.133: "Avoid `protected` data" of the C++ Core Guidelines. - Removed two unused (protected) data members.
|
This is now being reopened as is required by several projects. |
rfaasse
requested changes
Oct 6, 2023
There was a problem hiding this comment.
The code itself is quite clear, so most comments I have are minor changes to the structure and naming that can be done very quickly (~minutes).
However, there are three topics that will require more work:
- The inheritance direction of these two classes is the wrong way around. The 'Constant' version is a more specific version of the 'normal' one, so 'Constant' should inherit from 'normal', not the other way around as it is now. This is the biggest item of this list.
- The changes made to the apply_scalar_constraint_table_process should be moved to the C++ equivalent after these changes are merged to master.
- The .hpp files for the processes should be split into .h and .cpp. This is not a lot of work, but slightly more than trivial.
I know this code was for a large part copied from earlier development, but since it's new code, adding it to master in the current state would add to our technical debt. Therefore, I'd propose that we first spend some time to improve it.
...echanicsApplication/custom_processes/apply_constant_phreatic_multi_line_pressure_process.hpp
Outdated
Show resolved
Hide resolved
...echanicsApplication/custom_processes/apply_constant_phreatic_multi_line_pressure_process.hpp
Outdated
Show resolved
Hide resolved
...echanicsApplication/custom_processes/apply_constant_phreatic_multi_line_pressure_process.hpp
Outdated
Show resolved
Hide resolved
...echanicsApplication/custom_processes/apply_constant_phreatic_multi_line_pressure_process.hpp
Outdated
Show resolved
Hide resolved
...echanicsApplication/custom_processes/apply_constant_phreatic_multi_line_pressure_process.hpp
Outdated
Show resolved
Hide resolved
...eoMechanicsApplication/custom_processes/apply_phreatic_multi_line_pressure_table_process.hpp
Outdated
Show resolved
Hide resolved
...echanicsApplication/custom_processes/apply_constant_phreatic_multi_line_pressure_process.hpp
Outdated
Show resolved
Hide resolved
...eoMechanicsApplication/custom_processes/apply_phreatic_multi_line_pressure_table_process.hpp
Outdated
Show resolved
Hide resolved
...eoMechanicsApplication/custom_processes/apply_phreatic_multi_line_pressure_table_process.hpp
Outdated
Show resolved
Hide resolved
carloslubbers
added
the
GeoMechanics
carloslubbers
changed the title
…moved duplicate code
…onstraints table process
…'s no longer needed
rfaasse
reviewed
Oct 13, 2023
applications/GeoMechanicsApplication/python_scripts/apply_scalar_constraint_table_process.py
Outdated
Show resolved
Hide resolved
rfaasse
previously approved these changes
Oct 18, 2023
mnabideltares
previously approved these changes
Oct 19, 2023
rfaasse
approved these changes
Oct 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📝 Description
Added phreatic multi line both statically (constant) and transiently (table) processes
🆕 Changelog
Closes #10290