Updating Doc String: Address Issue #258 #263
Conversation
Updating the docstings in Zapdos is in progress with the following objects completed: - Actions - Auxkernels - bcs - constraits - indicators - interfacekernels - postprocessors - userobjects Also minor edits of head and body files were done, which involved removing out dated parameters
|
The PR for updating Zapdos Doc Sting is ready for review. @gsgall is the primary reviewer, but I included @cticenhour in case he would like to look at this PR too. |
|
Job Documentation, step Sync to remote on fb79626 wanted to post the following: View the site here This comment will be updated on new commits. |
There was a problem hiding this comment.
Overall looks good, a few typos and some missing documentation, and lack of detail in comments but thank you for doing this @csdechant.
| /* | ||
| * Helper function that supplies the Kernels for drift-diffusion for the electrons, | ||
| * energy independent charged particles, neutral particles, and | ||
| * electron mean energy depending |
There was a problem hiding this comment.
This sentence seems incomplete. Is this suppose to be mean electron energy dependent particles?
There was a problem hiding this comment.
It should read "electron mean energy density". Will edit.
There was a problem hiding this comment.
This has been addressed in 4b5a202
| virtual void addADKernels(const std::string & name, | ||
| const std::string & potential_name, | ||
| const bool & Using_offset, | ||
| const bool & charged, | ||
| const bool & energy); | ||
|
|
||
| /// Helper function that supplies the Aux kernels to convert scaled position units |
There was a problem hiding this comment.
What are the scaled position units in this case referring to? Is this for when the user is using a non-dimensioqnalized spatial domain?
There was a problem hiding this comment.
That is correct. For clarity, I can add edit to:
/// Helper function that supplies the Aux kernels to convert scaled position units when parameter position_units is set to non-unity
There was a problem hiding this comment.
This has been addressed in 4b5a202
| Real _period; | ||
| /// The number of cycles to calculate the difference |
There was a problem hiding this comment.
This description is unclear to me. Can you add some more detail here?
There was a problem hiding this comment.
Sure, a probably better description is "The number of cycles PeriodicRelativeNodalDifference is active"
There was a problem hiding this comment.
This has been addressed in 4b5a202
| @@ -56,13 +56,12 @@ AddPeriodicRelativeNodalDifference::validParams() | |||
| params.addParam<Real>( | |||
| "starting_cycle", 0.0, "The number of the cycles before starting the difference calculation"); | |||
| params.addRequiredParam<Real>("cycle_frequency", "The cycle's frequency in Hz"); | |||
| params.addParam<Real>( | |||
| "num_cycles", 2000.0, "The number of cycles to calculation the difference for."); | |||
| params.addParam<Real>("num_cycles", 2000.0, "The number of cycles to calculate the difference."); | |||
There was a problem hiding this comment.
This is a bit unclear to me. Does this mean the number of cycles for which the difference between the current and previous cycle is calculated. Or is this something like the number of cycles between cycles that are being compared?
There was a problem hiding this comment.
It is the former description. I can edit it to simply say "The number of cycles this object is active".
There was a problem hiding this comment.
This has been addressed in 4b5a202
|
|
||
| /* | ||
| * Helper function that supplies the Kernels for drift-diffusion for the electrons, | ||
| * energy independent charged particles, neutral particles, and | ||
| * electron mean energy depending | ||
| */ | ||
| virtual void addADKernels(const std::string & name, | ||
| const std::string & potential_name, | ||
| const bool & Using_offset, | ||
| const bool & charged, | ||
| const bool & energy); | ||
|
|
||
| /// Helper function that supplies the Aux kernels to convert scaled position units | ||
| virtual void addPosition(const std::string & position_name, const int & component); | ||
|
|
||
| /// Helper function that supplies the Aux kernels to convert densities from log form | ||
| virtual void addDensityLog(const std::string & particle_name); | ||
|
|
||
| /// Helper function that supplies the Aux kernels for current | ||
| virtual void addCurrent(const std::string & particle_name, const std::string & potential_name); | ||
|
|
||
| /// Helper function that supplies the Aux kernels for the electric field | ||
| virtual void addEfield(const std::string & Efield_name, | ||
| const std::string & potential_name, | ||
| const int & component); |
There was a problem hiding this comment.
@cticenhour correct me if I am wrong but I believe the MOOSE standard should be to have a description for each of the parameters of every custom function. In the style of
@param name description
This comment is also applicable for the rest of the actions that do not have the parameter doc strings for their internal functions.
There was a problem hiding this comment.
This is true for most MOOSE header files I have seen and should add them regardless for better documentation. This was an oversight and will edit.
There was a problem hiding this comment.
This has been addressed in 4b5a202
Co-authored-by: Grayson Gall <66559200+gsgall@users.noreply.github.com>
|
Job Precheck, step Clang format on 6a94f37 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:
|
|
@gsgall everything should be addressed now, and this PR is ready for a re-review. |
This PR updates the Zapdos header files to include previously missing Doc String, which will close Issue #258. This PR also includes minor edits that remove previously unused parameters in Zapdos objects.