First round of review changes #2
Conversation
|
@hchen139 As I'm going through this code, I think it will be easier for me to just make the changes in many cases, so I'd like to try this approach out. I can submit proposed changes, and you can merge them into your branch if you agree with them. |
| @@ -20,7 +20,7 @@ validParams<BondStatusPD>() | |||
| params.addClassDescription("Class for updating the bond status based on different failure " | |||
| "criteria: critical stretch and " | |||
| "maximum principal stress"); | |||
| MooseEnum FailureCriteriaType("Critical_Stretch Maximum_Tensile_Stress", "Critical_Stretch"); | |||
There was a problem hiding this comment.
A comment here: I changed these names to be more consistent with MOOSE conventions. I expected to break a test when I did that, but this is apparently not tested.
There was a problem hiding this comment.
This code has not been tested, i.e., there is no corresponding regression test. We don't really have a test for crack propagation problem.
| @@ -16,7 +16,7 @@ InputParameters | |||
| validParams<MechanicalStretchPD>() | |||
| { | |||
| InputParameters params = validParams<AuxKernelBasePD>(); | |||
| params.addClassDescription("Class for outputing bond mechanical stretch value"); | |||
| params.addClassDescription("Class for outputing bond mechanical stretch"); | |||
| params.set<ExecFlagEnum>("execute_on") = EXEC_TIMESTEP_END; | |||
There was a problem hiding this comment.
Can't you just use MaterialRealAux for this purpose? Also, I'm a little hesitant about setting a nonstandard value for 'execute_on' -- if the user wants it just for output purposes, I think I would expect them to set this.
There was a problem hiding this comment.
The MaterialRealAux outputs element volume-averaged material properties. There are some calculations related to volume and I'm sure whether they can be directly applied to Edge2 element. You are right, since the purpose of this AuxKernel is to output the bond mechanical stretch, it should be the user's decision to set the output frequency as they want.
| enum class FailureCriterion | ||
| { | ||
| CriticalStretch, | ||
| MaximumTensileStress |
There was a problem hiding this comment.
The correct available failure criterion name should be CriticalStretch and MaximumPrincipalStress. This is a bug in the original code.
| @@ -20,7 +20,7 @@ validParams<BondStatusPD>() | |||
| params.addClassDescription("Class for updating the bond status based on different failure " | |||
| "criteria: critical stretch and " | |||
| "maximum principal stress"); | |||
| MooseEnum FailureCriteriaType("Critical_Stretch Maximum_Tensile_Stress", "Critical_Stretch"); | |||
There was a problem hiding this comment.
This code has not been tested, i.e., there is no corresponding regression test. We don't really have a test for crack propagation problem.
| @@ -20,7 +20,7 @@ validParams<BondStatusPD>() | |||
| params.addClassDescription("Class for updating the bond status based on different failure " | |||
| "criteria: critical stretch and " | |||
| "maximum principal stress"); | |||
| MooseEnum FailureCriteriaType("Critical_Stretch Maximum_Tensile_Stress", "Critical_Stretch"); | |||
| MooseEnum FailureCriteriaType("CriticalStretch MaximumTensileStress", "CriticalStretch"); | |||
There was a problem hiding this comment.
Again, we need to use MaximumPrincipalStress rather than MaximumTensileStress.
| break; | ||
|
|
||
| case 1: | ||
| case FailureCriterion::MaximumTensileStress: |
There was a problem hiding this comment.
FailureCriterion::MaximumPrincipalStress
| val = _mechanical_stretch[0]; | ||
| break; | ||
|
|
||
| case 1: | ||
| case FailureCriterion::MaximumTensileStress: |
There was a problem hiding this comment.
FailureCriterion::MaximumPrincipalStress
| @@ -16,7 +16,7 @@ InputParameters | |||
| validParams<MechanicalStretchPD>() | |||
| { | |||
| InputParameters params = validParams<AuxKernelBasePD>(); | |||
| params.addClassDescription("Class for outputing bond mechanical stretch value"); | |||
| params.addClassDescription("Class for outputing bond mechanical stretch"); | |||
| params.set<ExecFlagEnum>("execute_on") = EXEC_TIMESTEP_END; | |||
There was a problem hiding this comment.
The MaterialRealAux outputs element volume-averaged material properties. There are some calculations related to volume and I'm sure whether they can be directly applied to Edge2 element. You are right, since the purpose of this AuxKernel is to output the bond mechanical stretch, it should be the user's decision to set the output frequency as they want.
|
@bwspenc Agree. Let's work on making changes on my local branch rather than submit small changes directly to MOOSE. |
|
I merged this pull request so that I can work on to address those issue myself. |
…derivative_test.i but with AD. Agrees well (#2)
ref idaholab#11561