-
Notifications
You must be signed in to change notification settings - Fork 249
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
Updating Poromechanics analysis and solver to new standard #2360
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Some comment but otherwise I am fine with the changes
I suggest that someone who know PoroMechanics approves this
import KratosMultiphysics.mpi as KratosMPI | ||
|
||
# Check that applications were imported in the main script | ||
KratosMultiphysics.CheckForPreviousImport() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this fct is currently not working, right @RiccardoRossi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recommended solution is to call KratosMultiphysics.CheckRegisteredApplications("...") instead.
@@ -0,0 +1,127 @@ | |||
from __future__ import absolute_import, division #makes KratosMultiphysics backward compatible with python 2.6 and 2.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks clean 👍
from python_solver import PythonSolver | ||
|
||
# Check that applications were imported in the main script | ||
KratosMultiphysics.CheckRegisteredApplications("PoromechanicsApplication") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you only check for Poro? also other apps are being imported later ...
|
||
class UPwSolver(PythonSolver): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a brief class description wouldn't hurt I guess
|
||
def AddDofs(self): | ||
# Set ProcessInfo variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this done in the basclass of the AnalysisStage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you point me out where in the AnalysisStage are the KratosMultiphysics.TIME, KratosMultiphysics.DELTA_TIME, and the KratosMultiphysics.STEP initialized?
Probably I am missing something, but I can't find it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are initialized when first used ...
TIME
: https://github.com/KratosMultiphysics/Kratos/blob/master/kratos/python_scripts/analysis_stage.py#L119-L121
STEP
: In AdvanceTimeStep
of the solver
DELTA_TIME
: inside ProcessInfo
when CloneTImeStep
is called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @philbucher !
In any case, I prefer to initialize those variables before filling the buffer and initializing the strategy...
|
||
# solve :: sequencial calls | ||
def Solve(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be called according to riccardo
The FluidSolver issues a warning here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I also added a warning.
builder_and_solver, | ||
self.strategy_params, | ||
max_iters, | ||
compute_reactions, | ||
reform_step_dofs, | ||
move_mesh_flag) | ||
else: | ||
solver = KratosPoro.PoromechanicsRammArcLengthStrategy(self.main_model_part, | ||
scheme, | ||
solver = KratosPoro.PoromechanicsRammArcLengthStrategy(self.computing_model_part, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this name should not be "solver" (the class is already called solver) => it is a solving_strategy
In StructuralMechanics we use "mechanical_solving_strategy" for it
|
||
# Execute the Metis partitioning and reading | ||
TrilinosModelPartImporter.ExecutePartitioningAndReading() | ||
self.TrilinosModelPartImporter.ExecutePartitioningAndReading() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this fct is deprecated, it is now called "ImportModelPart" => #2285
bcs it can now also do restart
@@ -224,9 +184,9 @@ def _ConstructSolver(self, builder_and_solver, scheme, convergence_criterion, st | |||
if strategy_type == "Newton-Raphson": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not for this PR but according to the Style Guide the name should be newton_raphson
=> also at other places
return FractureUPwSolver(main_model_part, custom_settings) | ||
|
||
|
||
class FractureUPwSolver(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does not derive from PythonSolver ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see it is kept for retro-compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is not up to current standards but in order to to adapt it, a complete modification in the fracture propagation utility would be necessary and I don't have the time to do so.
These changes are the minimal so that it still works...
This is the PR that replaces #2121, taking into account last changes in PR #2135.
New:
Updated:
Outdated, but maintained for retro-compatibility:
Other files were modified for compatibility with the previous changes.