Multiple Ionic Species in BCs about zapdos HOT 21 CLOSED

That's ok. The only thing to note is that with these fixes, the gold files need to be re-golded, but the 2D tests are just the first few RF cycles of a coarse version of the GEC runs. Because of this, I am currently re-running the full simulations with the finer mesh to make sure everything works (which takes a day or two on the hpc). I would still recommend to push the PR tomorrow, just noting that the full 2D runs need to be re-examined.

from zapdos.

Yeah, I figured the change would require re-golding. I made a minor fix to Hagelaar's secondary electron BC as well so I was planning on re-golding a few tests anyway.

from zapdos.

@lindsayad @cticenhour @csdechant Is there any reason why the _n_gamma parameter is commented out in the HagelaarEnergyBC and SecondaryElectronBC? I've converted those BCs along with NeumannCircuitVoltageMoles_KV to allow a vector of ions to be input and I'm going to put together a PR for this soon, but I just wanted to figure out what's going on with this before I start committing changes.

EDIT: In fact, it looks like lots of things are commented out in SecondaryElectronBC. For example. the Jacobian term is set to zero even though it's not going to be zero (unless I'm missing something?). I wonder if it's just so small that it has no influence on the Newton solve. Unless there's a reason for this that I'm missing, I'm going to go ahead and uncomment these terms and correct the Jacobian in SecondaryElectronBC as part of this PR.

from zapdos.

_n_gamma parameter is commented out in the HagelaarEnergyBC

Unfortunately, git log is not very helpful here. This got commented out in d61cc09, which has the fairly unhelpful commit message of:

Building new kernels for rate processes with function description of rate coefficients for hopefully better convergence.

My best guess for why this happened is that I planned to split out the BC into multiple integrated bcs like I did for the electron density, e.g. something like HagelaarEnergyBC and SecondaryElectronEnergyBC. Of course _n_gamma is actually still present in the residual statement for HagelaarEnergyBC but it should always be zero since that is what it is initialized to in the constructor; regardless this inconsistency is very bad. It would be great if you cleaned it up/got the physics correct. You also made me realize that I have a typo in equation 3.20 of my thesis. The secondary electron flux term is missing a 2. / (1. + r) factor

_n_gamma is commented out in SecondaryElectronBC

It is not commented out in the residual, but yes it is commented out in the Jacobian. Definitely some loss in the accuracy of the Jacobian, but it must be that the residual is dominated by the ion flux since I generally have always seen quadratic convergence in Newton's method. Still wouldn't hurt to get it completely right though. Might be a good time to switch it over to automatic differentiation as well...

from zapdos.

Alright, I've implemented new energy boundary conditions and fixed SecondaryElectronBC. I decided to go ahead and separate the HagelaarEnergyBC in two (HagelaarEnergyBC and SecondaryElectronEnergyBC) because otherwise it would be calculating a secondary electron energy contribution from a surface that may not have any secondary electron emission applied. Of course, it doesn't really change the results much but I think it's more realistic this way. I also added the 2./(1.+r) factor to both secondary electron BCs.

With these changes, though, the 1d_dc tests (1d_dc and 1d_dc_multispecies) fail since _n_gamma is included and the ion flux was taken out of HagelaarEnergyBC. Would you all prefer that I create new BCs to prevent this, or is anyone opposed to me just updating the tests? I've never had a test conflict before so I don't know what the most appropriate solution is.

Might be a good time to switch it over to automatic differentiation as well...

I completely agree after this experience. I spent a long time debugging a couple small jacobian errors. Using AD would make future development much easier. I'm going to devote some time to converting everything this week.

from zapdos.

from zapdos.

Yeah the results are qualitatively the same. The densities have about 1%-6% difference compared to the current gold values, but the shape is pretty much identical. I'll go ahead and regold them.

from zapdos.

from zapdos.

I'm working on adding multiple ions to Lymberopoulos and Sakiyama BCs as part of the SurfaceCharge PR I'm putting together, but I noticed something strange. @csdechant is there a reason why std::exp(_u[_qp]) is used in line 114 of SakiyamaSecondaryElectronBC? Unless I misunderstand what's going on here, that should be std::exp(_ip[_qp]), shouldn't it?

from zapdos.

No, you are right. I was actually in the middle of re-running some 2D GEC cases for a Zapdos paper and notice some errors I made in the 2D boundary conditions. I am fixing them right now, plus I need to reformat their header files to the newer MOOSE format. The updates fixes will be in my "debug" branch and should be done later today/tomorrow. Sorry for the errors and inconvenience. If you want, I can include the option for multiple ions for those BCs along with the fixes.

from zapdos.

No, that's alright -- I'm already halfway done with adding multiple ions so I'll just finish it up myself! Just wanted to make sure I was updating this properly.

from zapdos.

@csdechant I saw your changes. This PR is getting a little big already, so I was thinking of splitting it into two - one for updating the BCs with multiple ions and your other fixes, and the other to add surface charge.

If it's okay with you, I'll add your BC fixes to mine and throw all of that into the first PR sometime tomorrow.

from zapdos.

@lindsayad @keniley1 @csdechant @cticenhour I believe my syntax update PR #223 has taken care of all of these except NeumannCircuitVoltageNew which is the only non-ad object left.

Do any of you have a preference about what should be done with this BC (leave it alone, convert to AD and multiple ions or potentially deprecate)?

from zapdos.

@gsgall you mean #226? NeumannCircuitVoltageNew and PenaltyCircuitPotential are still non-AD, and PotentialDriftOutflowBC doesn't look like it needs modification at all (it doesn't take in ionic species as a parameter, only a single potential (and could be used multiple times if we were using effective potentials for the ions). I have updated the list to reflect this.

NeumannCircuitVoltageNew isn't even registered properly (so it isn't showing up in syntax lists), and this was never caught because its only uses are commented out in the tested input files. Without digging into the inputs at all, I would lean toward removing this object altogether.

PenaltyCircuitPotential still only uses a single ionic species, so this would still need to be fixed up for that component. It also does not appear that the NonlocalIntegratedBC base class has an AD version at all.

from zapdos.

Yes @cticenhour sorry about the wrong number there but yeah that is correct.

In PenaltyCircuitPotential it looks like the ions only come up on the off diagonal and non local off diagonal methods. Correct me if I am wrong but to support multiple ions for this object we just need to get an _ip_id for each ionic species and and add a check to see if the jvar matches any of the ion ids we have in the computeQpNonlocalOffDiagJacobian and computeQpOffDiagJacobian methods. Is that correct?

from zapdos.

You'd also need to get the dofIndices for each ionic species as with the single species case, but yes, I think that's broadly correct.

from zapdos.

@csdechant Do you see any reason not to remove the 'NeumannCircuitVoltageNew` boundary condition?

from zapdos.

I personal never used NeumannCircuitVoltageNew, but looking at the body file, it seems to be an earlier version of NeumannCircuitVoltageMoles_KV expect the current is formulated with a UserObject instead of directly inside the BC object. I don't see a reason to keep it, since NeumannCircuitVoltageMoles_KV exists.

from zapdos.

@csdechant @cticenhour I just submitted a PR removing the files and with that I believe this issue should be able to be closed.

from zapdos.

Just reviewed, with some requested additions. Thanks!

from zapdos.

@cticenhour Just took care of the changes

from zapdos.