Skip to content
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

Merging Casey's Thesis Work #265

Open
wants to merge 81 commits into
base: devel
Choose a base branch
from

Conversation

csdechant
Copy link
Collaborator

This PR directly addresses and should close Issue #257. This PR is based on @cticenhour thesis-rebase branch, excluding $H_{\phi}$ objects. The main focus of this PR includes:

  • The includes of a new material object, FieldSolverMaterial, which allows uses to supply the electric field terms in the drift-diffusion equation as a electrostatic potential variable or a vector electric field variable.
  • A material object to calculate the plasma dielectric coefficient.

This PR also includes updates based on @csdechant ZapdosAD-plus (which was rebased early on into the thesis-rebase branch), the excluding FV objects. That work includes:

  • Kernel objects for the electron energy flux using a thermal conductivity and an effected electric field calculation for ions.
  • Boundary conditions objects using an effected electric field calculation for ions and a dielectric time integral boundary condition.
  • Tests based on the method of manufactured solutions (MMS).
  • Minor bug fixes to the Newton-Shooting method acceleration objects.

@csdechant
Copy link
Collaborator Author

@cticenhour this PR has a lot of commit to begin with. I am in favor of squashing all the commit into one, but since this PR is based on your old branch, I wanted to get your opinion first.

@csdechant
Copy link
Collaborator Author

@cticenhour I am fixing the Precheck errors and one of them is a "banned keywords" error. The PlasmaDielectricConstant material object uses std::cout. I am suggest that I just comment out the section of code and leave a NOTE: comment stating that users can uncomment this section of code for debugging purposes. What are your thoughts?

@csdechant csdechant force-pushed the Casey-Thesis-No-HPhi-Objects branch from 7fa241a to a8a7474 Compare November 22, 2024 23:03
@moosebuild
Copy link
Collaborator

Job Precheck, step Clang format on 7014a11 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/zapdos/docs/PRs/265/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 402e17523a5ef51b0dac40c3c8175cd976b6ce48

@moosebuild
Copy link
Collaborator

moosebuild commented Nov 22, 2024

Job Documentation, step Sync to remote on 8300f40 wanted to post the following:

View the site here

This comment will be updated on new commits.

@csdechant
Copy link
Collaborator Author

@cticenhour @gsgall This PR is ready for review. The same as for the other active PR, #263, @gsgall is planned to be the primary reviewer (since this PR involved Casey's and my work). There are two issues I want to point out:

  • Depending on which PR is merged first (this one or Updating Doc String: Address Issue #258 #263), the other PR will most likely need to be rebased due to replacing _grad_potential with _electric_field.
  • There is a test fail due to a Exodiff for tutorial/tutorial05-PlasmaWaterInterface. In particular, the difference is with the Current_OHm and EFieldx1 auxvariables with a relative difference of 3.46855e-05 and 1.67417e-05, respectively. This error only occurs with CIVET and I have not been able to replicated with my Mac (Intel processor). I am in favor of increasing the ceiling threshold for these variables, but I wanted you guys input first.

…jects

Due to the git history involving a mixture of edits between the HPhiCylindricalPlasma and PlasmaDielectricConstant kernel objects and the PlasmaDielectricConstant material object, there is no clear breaking point the only includes the PlasmaDielectricConstant material object without substantial re-work. For this reason, the kernel objects will be removed from this branch and worked in the based branch until further notice.
Removing the FaradayCurrentBC object since it belongs to the HPhiCylindricalPlasma set of objects. These objects will be work on in a seperate branch.
AddPeriodicControllers and Shooting Method Kernels were updated in previous commits
(868f397 and 2c20476, respectively)
which resulted in changes in the tests that involve these objects. This commit addresses
those test changes.
@gsgall
Copy link
Collaborator

gsgall commented Nov 27, 2024

@csdechant Is the testing strategy of this PR similar to the larger physics based testing used for the majority Zapdos testing? Or is it more math based, focusing on the simplest tests possible for each kernel, i.e having a single kernel in an input whenever possible.

@gsgall All new test are math based (MMS to be exact) and in my option, all new tests in Zapdos should be against some type of known solution (either analytical or MMS) and any new test based on validation should be on a case-by-case basis. For the new MMS test, they are designed to test problems of increase coupling (i.e. one for diffusion-only, next with advection-diffusion with a function potential, next with advection-diffusion with variable potential, etc.).

The reason for the large gold files is that these MMS tests use solutions designed for an RF discharge, so the test are for one RF cycle. The current tests looks at every time step, so the gold files are big (in honesty, I could probably increase the step size too, as the original step size was chosen to be significantly small enough not to interfere with spatial convergence studies).

@csdechant Is there a reason we cannot just a steady state mms test for each Kernel individually? Testing with increasing coupling over an entire RF cycle seems like it is overkill for testing each Kernel and a bit redundant. I don't really see a need to have these tests be a transient simulation if the Kernel is not a time derivative. If we guarantee that each kernel works as expected on its own we should be able to guarantee the expected behaviour of our system, since MOOSE takes care of using multiple kernels in one system. I would like to advocate for a simpler testing approach. A single steady state test MMS test for each kernel where each test uses only as many objects as strictly necessary for testing a specific kernel, ideally a single kernel per input, if possible. I would also like to see our testing convention be something closer to the MOOSE convection of separating the tests by what type of object we are testing (Kernel, AuxKernel, ..., etc.).

The other concern, along with the size of gold files, I have with these tests is the amount of time it takes to run the test suite. Checking out the CI, testing, https://civet.inl.gov/job/2565410/, it shows that this test suite takes a little over 7 minutes to run. It also shows an average test time of 16 seconds, even when running the tests in parallel. I really think we should be aiming to decrease this to somewhere around a few seconds for each test, unless there is a specific reason we need these extended tests.

@csdechant csdechant force-pushed the Casey-Thesis-No-HPhi-Objects branch from c37b2a6 to c17657f Compare November 27, 2024 22:57
@csdechant
Copy link
Collaborator Author

@csdechant Is there a reason we cannot just a steady state mms test for each Kernel individually? Testing with increasing coupling over an entire RF cycle seems like it is overkill for testing each Kernel and a bit redundant. I don't really see a need to have these tests be a transient simulation if the Kernel is not a time derivative. If we guarantee that each kernel works as expected on its own we should be able to guarantee the expected behaviour of our system, since MOOSE takes care of using multiple kernels in one system.

These MMS test were to verify Zapdos for the uses of RF discharges, so the manufactured solution should represent a simple, yet similar, solution of a RF discharge. The reason for that is one should not assume that since two separate parts of codes work as intended, that the coupled version will work and shouldn't be tested. This is similar to the idea between unit vs integration testing. While MOOSE combines the residuals and handles the Jacobians of multiple kernels, that does not ensure that the coupled model will behave as expected (just an example, what if a developer coded a variable or material as non-AD when it should be AD? The non-couple test will show no error, as there is no coupling of variables, and MOOSE doesn't know it should be AD, MOOSE will just provide an incorrect Jacobian).

I would like to advocate for a simpler testing approach. A single steady state test MMS test for each kernel where each test uses only as many objects as strictly necessary for testing a specific kernel, ideally a single kernel per input, if possible. I would also like to see our testing convention be something closer to the MOOSE convection of separating the tests by what type of object we are testing (Kernel, AuxKernel, ..., etc.).

I agree and disagree with this to a point (maybe it is just wording). My view is similar, as Zapdos should have more simple cases (similar to the MOOSE framework), but also my rigid coupled cases (similar to other MOOSE modules, such as the Navier Stokes module). My vision for the Zapdos testbed is as such:

  • Have a type of "unit test" system that test just one kernel at a time (This is not a "true" unit test, as these are not testing single functions). These would be strictly non-heavy tests.
  • Have a type of integration test system that test the coupling of multiple kernels and actions to verify the intended models of Zapdos. This would be a mixture of non-heavy and heavy tests, depending on run time. These should be coarse version of more rigid verification tests.
  • Have a system of verification tests. These are full convergence study test that compare the CSV files output and plot convergence slopes. These are marked as heavy, by default, and will generate verification/convergences plots for the Zapdos website (similar to TMAP8).
  • Finally, have a system of validation tests. Similar to the verifications test, these test will be marked as heavy, by default, and if need will be marked as hpc-heavy. These tests too will generate plots for the Zapdos website (similar to TMAP8 validation tests).

The other concern, along with the size of gold files, I have with these tests is the amount of time it takes to run the test suite. Checking out the CI, testing, https://civet.inl.gov/job/2565410/, it shows that this test suite takes a little over 7 minutes to run. It also shows an average test time of 16 seconds, even when running the tests in parallel. I really think we should be aiming to decrease this to somewhere around a few seconds for each test, unless there is a specific reason we need these extended tests.

I just push a commit to reduce the gold files and run time. Also, the new MMS test are marked as heavy, so they will only run if a user inputs ./run_tests -jn --heavy. It seems CIVET for Zapdos runs all the test together instead as a separate Test Heavy, like for MOOSE and MOOSE module PRs. @cticenhour Is there a setting to separate regular vs heavy test, so it doesn't seem like develops are submitting longer test without heavy = True?

@gsgall Please let me know if you have any question or different options as what I have stated above. @cticenhour We talked about this a little offline about revamping the Zapdos testbed, in regards to Issue #261. Please let my know if you have any comments or concerns about the about vision for the Zapdos testbed.

Copy link
Collaborator

@gsgall gsgall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general most of my comments are small changes.

There are a few things that I need to be address but this PR may not be the right place for them. But, here is a summary of those changes:

  • There are a lot of files in this PR which do not adhere to const correctness.
  • Some files have MooseVariables as class member variables but only use those during construction. Unless there is a good reason those should not be member variables but temporary constructor variables for the class.
  • A lot of header files are missing proper documentation for all variables, functions and function parameters.
  • Some of the files which add new functionality do not fully support multi-ion systems. Nor do they use materials for secondary emission coefficients like the rest of the current Zapdos implementation.

doc/content/source/auxkernels/Current.md Outdated Show resolved Hide resolved
doc/content/source/auxkernels/DriftDiffusionFluxAux.md Outdated Show resolved Hide resolved
doc/content/source/auxkernels/EFieldAdvAux.md Outdated Show resolved Hide resolved
doc/content/source/auxkernels/EFieldAdvAux.md Outdated Show resolved Hide resolved
doc/content/source/auxkernels/PowerDep.md Outdated Show resolved Hide resolved
mooseError("There are electrons or charged_particles that are missing their potential! Please "
"check your input.");
if (!field_present && (em_present || (number_ions > 0)))
mooseError(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably be more informative if we used a paramError instead of a mooseError

Comment on lines 174 to +175
mooseError("There are secondary_charged_particles that are missing their corresponding "
"effective potential (eff_potentials)! Please check your input.");
"effective fields (eff_fields)! Please check your input.");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar suggestion here, using paramError rather than a mooseError.

}

// If control cycles are back to back, then disable_start and disable_end times would be the
// same, To avoid this, if cycles are back to back, then the disable_end starts at +
// 0.0001*period
if (_cycles_per_controls == 1.0)
if (_cycles_between_controls == 1.0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really shouldn't be doing this sort of float equality comparison. we should be using the absolute fuzzy equals functionality for this sort of comparison.

Comment on lines +43 to +63
mooseError("Solver mode is electrostatic, but electric field was provided. Please either "
"change the mode or provide a potential variable.");
}
else if (_mode == ELECTROMAGNETIC && parameters.isParamSetByUser("potential") &&
!parameters.isParamSetByUser("electric_field"))
{
mooseError("Solver mode is electromagnetic, but potential was provided. Please either change "
"the mode or provide an electric field variable.");
}
else if (parameters.isParamSetByUser("electric_field") &&
parameters.isParamSetByUser("potential"))
{
mooseError("Both electric field and potential variables have been provided. Please provide "
"only one, consistent with the solver mode parameter.");
}
else if (!parameters.isParamSetByUser("electric_field") &&
!parameters.isParamSetByUser("potential"))
{
mooseError("Neither potential nor electric field variables are defined. Please provide a "
"variable for field determination.");
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be using paramError instead of mooseErrors.

Comment on lines +338 to +339
_EField[_qp] = _electric_field[_qp](0); // TODO: does this need to be 0 component now? consider
// fixing this up....1D will request the 0 by default
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these going to be addressed in this PR or a later one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This object was made during Alex's thesis and I haven't used it except when I was first using Zapdos. After a quick search in Zapdos, it doesn't seem that any of the test files used this exact material property. That being said, I don't think we should remove it for this PR, as this and other material objects (such as Gas.C and GasElectronMoments) should be re-examined to see if any declare property are not being utilized in a current test and address the need for a test or the removal of the property.

@csdechant
Copy link
Collaborator Author

@gsgall I am working on this PR this week, but I did want to address one of your comments right now:

  • A lot of header files are missing proper documentation for all variables, functions and function parameters

Are you referring to any of the newly added header files or the previous header files? If it is the latter, that falls under PR #263 (which is ready for re-review). Depending on which PR is merged first, the other PR will most likely need to be rebased due to replacing _grad_potential with _electric_field. If possible, I would like PR #263 to be merged first.

Copy link
Collaborator Author

@csdechant csdechant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsgall I am working on addressing your comments, but I have addressed some of your questions regarding if a change should be made in this PR or a separate PR (as I believe some of the comment should be address in a separate PR, in the spirit of small but more focused PR in Zapdos' future). Please let me know if you have a difference opinion on which issues belong in this PR or a separate PR.

\end{cases} \\[10pt]
\Gamma_{j} \cdot \textbf{n} = \frac{1-r_{j}}{1+r_{j}} \left[ (2 a_{j}-1) \ \mu_{j}
\left(
\text{-} \nabla (V / l_{c})
\vec{E}V / l_{c}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a typo. $- \nabla (V / l_{c})$ was supposed to be replaced by $\vec{E} / l_{c}$. I will address this.

Comment on lines +2 to +20

!alert construction title=Undocumented System
The DriftDiffusionAction system has not been documented. The content listed below should be used as a starting
point for documenting the system, which includes the typical automatic documentation associated with
a system; however, what is contained is ultimately determined by what is necessary to make the
documentation clear for users.

## Overview

!! Replace this line with information regarding the DriftDiffusionAction system.

## Example Input File Syntax

!! Describe and include an example of how to use the DriftDiffusionAction system.

!syntax list /DriftDiffusionAction objects=True actions=False subsystems=False

!syntax list /DriftDiffusionAction objects=False actions=False subsystems=True

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation edits of previous objects fall under Issue #260 and should be address in a separate PR.
Also, this edit already exists in the current devel branch of Zados (link here). I am not sure why GitHub is showing the change this PR, as both the devel branch and this branch shows the change as the same commit.

Comment on lines +1 to +15
# PeriodicControllers System

!alert construction title=Undocumented System
The PeriodicControllers system has not been documented. The content listed below should be used as a starting
point for documenting the system, which includes the typical automatic documentation associated with
a system; however, what is contained is ultimately determined by what is necessary to make the
documentation clear for users.

## Overview

!! Replace this line with information regarding the PeriodicControllers system.

## Example Input File Syntax

!! Describe and include an example of how to use the PeriodicControllers system.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same commit regarding Issue #260

Comment on lines +1 to +14
# PeriodicRelativeNodalDifference System

!alert construction title=Undocumented System
The PeriodicRelativeNodalDifference system has not been documented. The content listed below should be used as a starting
point for documenting the system, which includes the typical automatic documentation associated with
a system; however, what is contained is ultimately determined by what is necessary to make the
documentation clear for users.

## Overview

!! Replace this line with information regarding the PeriodicRelativeNodalDifference system.

## Example Input File Syntax

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same commit regarding Issue #260

Comment on lines +26 to 46
virtual void addChargeSourceKernels(const std::string & field_name,
const std::string & charged_particle_name,
const MooseEnum & field_solver);
virtual void addADKernels(const std::string & name,
const std::string & potential_name,
const std::string & field_name,
const bool & Using_offset,
const bool & charged,
const bool & energy);
virtual void addPosition(const std::string & position_name, const int & component);

virtual void addDensityLog(const std::string & particle_name);
virtual void addCurrent(const std::string & particle_name, const std::string & potential_name);
virtual void addCurrent(const std::string & particle_name,
const std::string & field_property_name);
virtual void addEfield(const std::string & Efield_name,
const std::string & potential_name,
const std::string & field_property_name,
const int & component);
virtual void addFieldSolverMaterial(const std::string & field_name,
const std::string & field_property_name,
const MooseEnum & field_solver,
const bool & effective_field);
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edits to header file are address under the current PR #263. A rebase will need to be done to with ever PR is not approved first.

params.addCoupledVar("Ey", "The EField in the y-direction"); // only required in 2D and 3D
params.addCoupledVar("Ez", "The EField in the z-direction"); // only required in 3D

params.addParam<Real>("users_gamma",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds right. I believe this object pre-dates those changes and I just never updated this object due to the cases I was using it for (single ion species and constant secondary electron coefficient). I can update this object in this PR.

Comment on lines +37 to +67
DielectricBCWithEffEfield::DielectricBCWithEffEfield(const InputParameters & parameters)
: ADIntegratedBC(parameters),
_r_units(1. / getParam<Real>("position_units")),

_u_old(_var.slnOld()),
_grad_u_old(_var.gradSlnOld()),

_mean_en(adCoupledValue("mean_en")),
_mean_en_var(*getVar("mean_en", 0)),
_mean_en_old(_mean_en_var.slnOld()),
_em(adCoupledValue("em")),
_em_var(*getVar("em", 0)),
_em_old(_em_var.slnOld()),
_ip(adCoupledValue("ip")),
_ip_var(*getVar("ip", 0)),
_ip_old(_ip_var.slnOld()),

_Ex(adCoupledValue("Ex")),
_Ex_var(*getVar("Ex", 0)),
_Ex_old(_Ex_var.slnOld()),

//_Ey(adCoupledValue("Ey")),
_Ey(isCoupled("Ey") ? adCoupledValue("Ey") : _ad_zero),
_Ey_var(*getVar("Ey", 0)),
//_Ey_old(_Ey_var.slnOld()),
_Ey_old(isCoupled("Ey") ? _Ey_var.slnOld() : _zero),

//_Ez(adCoupledValue("Ez")),
_Ez(isCoupled("Ez") ? adCoupledValue("Ez") : _ad_zero),
_Ez_var(*getVar("Ez", 0)),
//_Ez_old(_Ez_var.slnOld()),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For contact the MooseVariable were needed the pre-AD days for defining the Jacobian matrix, but are now only used for calling slnOld. I never collapsed the variable after Zapdos went full AD. I can edit that in this PR.

Comment on lines +62 to +69
for (unsigned int i = 0; i < _num_ions; ++i)
{
_ip_var[i] = getVar("ip", i);
_ip[i] = &adCoupledValue("ip", i);
_muip[i] = &getADMaterialProperty<Real>("mu" + (*getVar("ip", i)).name());
_sgnip[i] = &getMaterialProperty<Real>("sgn" + (*getVar("ip", i)).name());
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a quick look at the other BC that account of multi-species ions, quite of few of them have those MooseVariables in the Devel branch due to the pre-AD days. Because of this, and in the spirit of having smaller but more focus PRs, I suggest we make this an issue and I can address this in a separate PR.

Comment on lines +24 to +25
params.addParam<Real>("users_gamma",
"A secondary electron emission coeff. only used for this BC.");
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the comment response to DielectricBCWithEffEfield.C and will address in this PR.

Comment on lines +338 to +339
_EField[_qp] = _electric_field[_qp](0); // TODO: does this need to be 0 component now? consider
// fixing this up....1D will request the 0 by default
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This object was made during Alex's thesis and I haven't used it except when I was first using Zapdos. After a quick search in Zapdos, it doesn't seem that any of the test files used this exact material property. That being said, I don't think we should remove it for this PR, as this and other material objects (such as Gas.C and GasElectronMoments) should be re-examined to see if any declare property are not being utilized in a current test and address the need for a test or the removal of the property.

@gsgall
Copy link
Collaborator

gsgall commented Jan 9, 2025

@gsgall I am working on addressing your comments, but I have addressed some of your questions regarding if a change should be made in this PR or a separate PR (as I believe some of the comment should be address in a separate PR, in the spirit of small but more focused PR in Zapdos' future). Please let me know if you have a difference opinion on which issues belong in this PR or a separate PR.

Thank you @csdechant. I think your comments and wanting to make these changes in a separate PR a reasonable. I don't have any issue with this plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants