Consistent naming conventions across all inputs about zapdos HOT 17 OPEN

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.

from zapdos.

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.

from zapdos.

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).

from zapdos.

Yeah that was my mistake. Thank you for adding them

from zapdos.

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).

from zapdos.

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.

from zapdos.

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.

from zapdos.

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.

from zapdos.

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.

from zapdos.

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?

from zapdos.

@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?

from zapdos.

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.

from zapdos.

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?

from zapdos.

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.

from zapdos.

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.

from zapdos.

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

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.

from zapdos.

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!

from zapdos.