-
Notifications
You must be signed in to change notification settings - Fork 42
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
Update parameter descriptions to use new API #397
base: devel
Are you sure you want to change the base?
Conversation
@@ -275,7 +278,7 @@ ConcreteASREigenstrain::computeResidual(unsigned qp, Real scalar) | |||
// Convert current temperature to Kelvin | |||
const Real T = _temperature[qp] + _temp_offset; | |||
|
|||
// ASR characteristic and latency times (in days) | |||
// ASR characteristic and latency times (in ) |
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.
Leave this one as it was. Same with the one below (Line 315)
"microcracking_initiation_strain", | ||
"microcracking_initiation_strain > 0", | ||
"Linear strain at which the microcracking initiates (in [m/m])"); | ||
// WGA - [m/m] ??? |
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.
Strain is unitless, so just don't even set it.
@@ -39,6 +39,7 @@ DamagePlasticityStressUpdate::validParams() | |||
"ft_ep_slope_factor_at_zero_ep", | |||
"ft_ep_slope_factor_at_zero_ep <= 1 & ft_ep_slope_factor_at_zero_ep >= 0", | |||
"slope of ft vs plastic strain curve at zero plastic strain"); | |||
// WGA - unit needed for this? |
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.
Damage is unitless. Stress and strain have units, but I believe we're flexible on what system the user chooses.
params.addCoupledVar("temperature", "Temperature variable [in Celsius]"); | ||
// WGA - using 0C for degrees celcius | ||
params.addCoupledVar("temperature", "Temperature variable"); | ||
params.setDocUnit("temperature", "0C"); |
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 we just use "C" for Celsius, but need to confirm.
"Latency ASR time (in days) at reference temprature (tau_L(T_0))"); | ||
"Latency ASR time at reference temprature (tau_L(T_0))"); | ||
params.setDocUnit("latency_time", "d"); | ||
// WGA - energy units? |
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 units for characteristic_activation_energy and latency_activation_energy are K. That's a little odd since they're temperature rather than energy units, so it's not really quite right to call them energies, but that's how the model is.
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'd also suggest adding these two in the appropriate locations. I'm not sure if the API will support this, but I want to capture that we need to deal with this:
params.setDocUnit("temperature", "Unit defined by 'temperature_units'");
params.setDocUnit("reference_temperature", "Unit defined by 'temperature_units'");
"Linear strain at which the microcracking initiates (in [m/m])"); | ||
params.addRequiredRangeCheckedParam<Real>("microcracking_initiation_strain", | ||
"microcracking_initiation_strain > 0", | ||
"Linear strain at which the microcracking initiates (in [m/m])"); |
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.
"Linear strain at which the microcracking initiates (in [m/m])"); | |
"Linear strain at which the microcracking initiates"); |
Strain is unitless, so defining it in m/m isn't really helpful in my opinion. Same goes for the microcracking_strain_branch
parameter below.
@@ -38,11 +38,15 @@ ConcreteLogarithmicCreepModel::validParams() | |||
1, | |||
"long_term_characteristic_time > 0", | |||
"Rate at which the long_term viscosity increases"); | |||
params.addCoupledVar("temperature", "Temperature variable [in Celsius]"); | |||
// WGA - using C for degrees celcius |
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.
// WGA - using C for degrees celcius |
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.
Also, some of the parameters above need units:
params.setDocUnit("youngs_modulus", "Model-specific stress unit");
params.setDocUnit("recoverable_youngs_modulus", "Model-specific stress unit");
params.setDocUnit("recoverable_viscosity", "Model-specific time unit");
params.setDocUnit("long_term_viscosity", "Model-specific time unit");
params.setDocUnit("long_term_characteristic_time", "Model-specific time unit");
Job Precheck on 6c53cdf wanted to post the following: Your code requires style changes. A patch was auto generated and copied here
Alternatively, with your repository up to date and in the top level of your repository:
|
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.
All of the prior comments that are not marked as resolved should still be addressed too.
@@ -24,12 +24,16 @@ ConcreteExpansionEigenstrainBase::validParams() | |||
"expansion_type", expansion_type, "Type of expansion resulting from volumetric strain"); | |||
params.addRangeCheckedParam<Real>( | |||
"compressive_strength", "compressive_strength > 0", "Compressive strength of concrete"); | |||
// WGA - unclear if units are needed? |
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.
Delete this comment now.
Co-authored-by: Ben Spencer <[email protected]>
Triggered by CIVET job https://civet.inl.gov/job/2312477
Triggered by CIVET job https://civet.inl.gov/job/2315029
Triggered by CIVET job https://civet.inl.gov/job/2320969
Triggered by CIVET job https://civet.inl.gov/job/2323264
Triggered by CIVET job https://civet.inl.gov/job/2325808
Triggered by CIVET job https://civet.inl.gov/job/2328699
Triggered by CIVET job https://civet.inl.gov/job/2330312
Triggered by CIVET job https://civet.inl.gov/job/2333183
Triggered by CIVET job https://civet.inl.gov/job/2334480
Co-authored-by: Ben Spencer <[email protected]>
Co-authored-by: Ben Spencer <[email protected]>
Co-authored-by: Ben Spencer <[email protected]>
Co-authored-by: Ben Spencer <[email protected]>
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 for fixing those up!
There are still a couple of changes to be made. Also, the moose and neml submodules shouldn't be modified by this PR. I can show you how to undo those.
params.addParam<Real>("cement_mass", "cement mass per m^3"); | ||
params.setDocUnit("cement_mass", "kg"); | ||
params.addParam<Real>("aggregate_mass", "aggregate mass per m^3"); | ||
params.setDocUnit("aggregate_mass", "kg"); | ||
params.addParam<Real>("water_to_cement_ratio", "water to cement ratio"); | ||
params.addParam<Real>("aggregate_vol_fraction", "volumetric fraction of aggregates"); | ||
params.addParam<Real>("concrete_cure_time", "concrete curing time in days"); |
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 change is still needed.
@@ -7,23 +7,23 @@ | |||
[shrinkage] | |||
type = 'Exodiff' | |||
input = 'shrinkage.i' | |||
# abs_zero = 1e-5 | |||
abs_zero = 2e-5 |
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 file shouldn't be changed. There was another commit that got merged in that modified these.
@@ -68,7 +68,7 @@ | |||
type = 'CSVDiff' | |||
input = 'concrete_expansion_microcracking.i' | |||
csvdiff = 'concrete_expansion_microcracking_5_5_out.csv' | |||
abs_zero = 1e-6 | |||
abs_zero = 2e-6 |
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 file shouldn't be changed either.
params.addRangeCheckedParam<Real>("latency_activation_energy", | ||
9400.0, | ||
"latency_activation_energy > 0.0", | ||
"Activation energy associated with latency_time (U_L)"); | ||
params.setDocUnit("latency_activation_energy", "K"); | ||
params.addRequiredParam<Real>("reference_temperature", | ||
"Reference temperature for ASR reaction constants."); |
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.
in unit specified by 'temperature_units'
ref #396
Using param.setDocUnit(...) to document the units of parameters that currently have units listed in their descriptions. This causes the units to show up in a separate part of the documentation page.