OneD Documentation improvements[OneD]#1757
Conversation
|
The new debug_sim1d.yaml file can be examined using the following script. |
There was a problem hiding this comment.
If possible, I'd avoid changing parts that affect regular 1D flames - this is production code and there are quite a few broken CI tests (due to a slight change of calculated flame speed).
Overall, it may be best to split changes into one PR that takes care of Cantera/enhancements#25 and another one that investigates #1747.
speth
left a comment
There was a problem hiding this comment.
Thanks for this contribution, @wandadars. The 1D code has definitely been in need of some documentation improvements.
Adding the descriptions of the finite difference terms is very useful, but I am concerned about ending up with too much repeated content, as appears here in the documentation for the dVdz, dYdz and dTdz methods, and again in shear and conduction. I think it would be best to describe the general approaches to upwinding and the second-order differences once, and then reference that description from the methods where those approaches are used. The expository style that you've written much of this in would be a good fit for the "reference" documentation, and there's even an existing placeholder page for just this type of information -- see doc/sphinx/reference/onedim/discretization.md. A description of the discretized 1D problem is a natural next step for the user to be interested in after seeing the governing equations, and I think it's useful not to have to jump all the way down into the weeds of the C++ implementation to find this.
I think most of the formatting changes here are an improvement. However, in the future, I'd recommend avoiding making widespread stylistic changes outside the specific functions that you're making substantive changes to, unless the formatting changes are the sole focus of your PR.
ischoegl
left a comment
There was a problem hiding this comment.
Solver changes should be separated from formatting/docstring updates.
I'll pull these changes apart into separate ones today. |
faf73f4 to
2ec1a7d
Compare
|
Ok. I think I got them separated properly. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1757 +/- ##
=======================================
Coverage 73.22% 73.23%
=======================================
Files 381 381
Lines 54344 54343 -1
Branches 9247 9246 -1
=======================================
Hits 39796 39796
+ Misses 11574 11573 -1
Partials 2974 2974 ☔ View full report in Codecov by Sentry. |
Thank you - this is much appreciated! |
ischoegl
left a comment
There was a problem hiding this comment.
For doxygen comments, I'd recommend succinct statements for the first sentence, which will become the "brief" description (via the JAVADOC_AUTOBRIEF setting, which Cantera sets to YES).
The brief description is what is shown as the one-line description of various sections / classes / functions, e.g. see example). Overly long descriptions lead to clutter in class and module listings.
|
This is a silly question, but is there a reason why we are upwinding on the energy equation term that is j_k* dh_k/dz? |
|
I added some notes to that nonlinear solver section too @speth . |
I think this is a historical artifact. This term used to be calculated in terms of the temperature, before @gkogekar updated it to generalize the calculation for non-ideal equations of state. Before that, this term used the |
|
What's the best way to preview the markdown pages? |
One way is to download artifacts from GH, under ‘Actions’, see here … download docs, unzip and open the compiled HTML documents in your browser. You could also run |
Thanks @ischoegl ! Man, that Sphinx dependency list is quite lengthy haha. Is the list of sphinx dependencies documented anywhere. For reference, it was: I was missing the "scons spinx" command. I only knew of the doxygen one from working on the high-pressure gas transport documentation. Being able to see the results is very helpful. |
Sure thing ... fwiw, here are conda environment recommendations, together with some instruction that @speth put together for the new documentation. |
|
Ok, I think the notes are in a state where they can be examined. |
speth
left a comment
There was a problem hiding this comment.
Thanks, @wandadars, this is very good step toward having a well-documented 1D solver. I had a number of suggestions for further improvements.
speth
left a comment
There was a problem hiding this comment.
Thanks, @wandadars. I think this is getting pretty close. Just some minor comments that should be easy to address.
| At the left boundary ($j=0$): | ||
|
|
||
| $$ | ||
| F_{u,j} = \frac{\rho_j u_j - \rho_{j+1} u_{j+1}}{z_{j+1} - z_j} + | ||
| 2 \left( \frac{\rho_j V_j + \rho_{j+1} V_{j+1}}{2} \right) | ||
| $$ |
There was a problem hiding this comment.
Likewise, this misses the burner stabilized case, where the equation is that
There was a problem hiding this comment.
Is this a default boundary condition though? Do we want to document all of those conditions as well here? I had originally planned to just do the ones that the flow domain imposed.
There was a problem hiding this comment.
Maybe it would be best to reserve that update for a separate PR, but I think the way the boundary conditions are implemented in the code, with the equations split between the flow domain and the connector, should be regarded as an implementation detail -- it's not really something that affects the mathematical formulation that's presented here.
There was a problem hiding this comment.
That makes sense. So essentially just discuss the primary discretization for the boundary conditions and flow. I think as long as the actual header documentation discusses the existence of a default set of boundary conditions, that would be good. From my personal experience, it was a bit confusing to figure out what was going on between the domain and the BC objects. Where a BC object might just add something to the residual vector, but it was clear what else was in that entry.
speth
left a comment
There was a problem hiding this comment.
Thanks, @wandadars. This looks good to me.
…ded 1D domain diagram svg image
…eady and unsteady solution process
…gram with consistent notation
In the course of trying to debug an issue with the oneD solver's two-point continuation, I thought that some clearer documentation would help.
Checklist
scons build&scons test) and unit tests address code coverage