Consistent naming conventions across all inputs

[gsgall]

Motivation

Currently the same input parameter across a majority of the Zapdos objects is scattered and not immediately obvious. I would like to propose the following standard for input parameter naming conventions.

Description	input
Electron density in log-molar form	electrons
Mean electron energy multiplied by electron density in log form	electron_energy
Multiple Ion Densities in log-molar form	ions
Ion Specific Potentials	ion_potentials
Species dependent secondary electron emission coefficient	emission_coeffs

If there are other common inputs missing from this list please let me know or propose a standard for those as well. Or if others would like to propose potential modifications to these standards I am open to feedback as well!

This issue goes into helping resolve some parts of #49 and #73 as well and improve the user experience by reducing the amount of input for certain cases by enabling the users to define these variables once with the GlobalParams block.

Solution

Deprecate all existing inconsistent input parameters from objects currently available in Zapdos with a removal date for those options set to be 01/01/2025

@lindsayad @csdechant @cticenhour What are your thoughts on adopting this naming convention standard?

[csdechant]

The naming convention suggested seems good to me. Also, I am including @cticenhour and @keniley1, in case they have some input (I saw that you included me twice in a row by mistake, so you might have meant to include them).

[gsgall]

Yeah that was my mistake. Thank you for adding them

[cticenhour]

The names you've chosen seem reasonable to me. The only suggestion I have is the deprecation date; I would prefer removing the old conventions sooner rather than later. So if we have a deprecation date, I would say 6 months instead of a year for this one (06/01/2024).

[gsgall]

That seems reasonable to me as well. I will plan to set the Depreciation date to 06/01/2024

The other question I wanted to bring up is what is the best way to handle this. Since this is going to involve changes to a lot of files I don't want to do this update all in one PR and drown the reviewers. Is there an ideal protocol for something like this? My initial thought on something like this would be to do groups of objects by type, BCs in one PR, Kernels in another and so on.

[csdechant]

In terms of PR, I believe this should just be one PR for the whole issue (so include all BCs, Kernels, Materials, etc.). If you want to reduce initial review time, maybe break up the commits into object groups and reviews can be conducted as commits are introduced. If need be, after the full review, all of the commits can then be squash into one commit for cleanup.

[gsgall]

I can certainly do that. Something else I just thought about was if these changes should be made as rolling changes so when one system is done we can send those changes for users while work is still being done on other systems. But I will defer to you guys on how this should be done.

[cticenhour]

My personal opinion is that transition PRs should be grouped by system. I personally prefer being able to review a complete, submitted and tested PR, rather than doing reviews as more commits are added. The PRs are smaller and more digestible. Once all systems have been converted over and deprecations are done, then we can work through the input conversions.

[csdechant]

I personally prefer being able to review a complete, submitted and tested PR, rather than doing reviews as more commits are added. The PRs are smaller and more digestible.

Fair enough. If the consensus is to break this up into multiple PRs, I have no objections.

[gsgall]

Multiple PRs sounds good to me.

For the test suite. I know this will require updating the input parameters of the test files. Should this be done with each PR or once the old options are finally removed? Or should there be additional tests to ensure that the deprecated inputs are still working until they are completely removed?

[cticenhour]

I would update the parameters in each PR, but don't yet touch the inputs. The tests should still pass.

Once all the changes are complete, you can run the test suite with ./run_tests -j8 --error-deprecated and you'll see every deprecation ready to be fixed at once. For that PR, we can also turn on a deprecated test target, which passes this same flag. When it is green, then we know we've got everything.

This, of course captures all the tested inputs. There may still be some untested ones that we'll have to make sure we get to manually, but this is usually captured in the normal find-and-replace fixups on the tested inputs.

[gsgall]

@cticenhour @csdechant I am currently going through the BCs system and I have noticed that quite a few of these boundary conditions do not mention the work that they came from in the class description. For those that do not have a DOI mentioned in their class description should they be added and if so, how can I find the publications that the BCs come from?

[csdechant]

I add the DOI to the BCs that I thought were novel enough (basically BCs you wouldn't just found in a text book). To found the DOI for the BCs that Alex used, I went though his papers & doctorial thesis, then went thought the references until I found the papers that derived the BCs.

Though I should mention that I am not sure if adding DOI for BCs is common within MOOSE and other MOOSE apps.

[gsgall]

In my opinion it could be a nice thing to have either in the class description parameter or on the documentation page for the boundary condition. As someone who is still relatively unfamiliar with some of the more standard boundary conditions a reference where I could read about the specific assumptions and regimes of applicability for each boundary condition could be nice for a new user as well. But I'll defer to you and Casey for what should be done with this sort of thing.

Additionally, I have found that the NeumannCircuitVoltageNew, and PenaltyCircuitPotential are also both Non-AD BCs should these be made AD boundary conditions as well?

[cticenhour]

The usual MOOSE approach here is to have a normal text description (maybe with some light latex, where applicable) with the references being placed in the documentation using the MooseDocs reference/bibliography system. You can find a description of that via the MooseDocs documentation page. If there isn't a zapdos.bib or similar located in the doc folder tree we should create one. I know I made some bib files for citations that use Zapdos, etc. but there should be a central one for text references.

Regarding non-AD objects, these probably should be converted. There might have been a reason I didn't do it during my AD sweep, but the reason escapes me at the moment.

[gsgall]

It looks like the only bib files are for the publications and dissertations/theses but I know @csdechant is working on documentation so that may also be something that is coming with that.

I can go ahead and update these BCs to AD during my syntax update as well then. Unless there is any other specific reason they should not be an AD object. This update will probably need to be made in a separate PR since the implementation looks like it was intentional and it may need some more discussion.

[gsgall]

I have also found another parameter which is inconsistent in some boundary conditions. The secondary electron energy has several different names. I would like to propose the following name

Description	input
Secondary Electron Energy	secondary_elctron_energy

I also think this change should be made since the secondary electron energy could be boundary dependent and currently this parameter is set by a magic number in a material object which really hides the assumption of this value from the users.

[lindsayad]

All sounds good to me. I wold probably have voted for one PR, but as I won't be reviewing it 😆 my vote shouldn't count for much!