Skip to content

OneD Documentation improvements[OneD]#1757

Merged
speth merged 11 commits intoCantera:mainfrom
wandadars:oneD_additions
Aug 18, 2024
Merged

OneD Documentation improvements[OneD]#1757
speth merged 11 commits intoCantera:mainfrom
wandadars:oneD_additions

Conversation

@wandadars
Copy link
Copy Markdown
Contributor

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

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@wandadars
Copy link
Copy Markdown
Contributor Author

The new debug_sim1d.yaml file can be examined using the following script.

import matplotlib.pyplot as plt
import yaml
import sys
import os
import re

'''
This script reads the debug output from Cantera when the loglevel is set to 8. the data that is
output is the history of the last 10 attempts of the solve() method's residual solution vectors.
'''

# Load YAML data
def load_yaml_data(file_path):
    """Load and return the YAML data from the given file path."""
    try:
        with open(file_path, 'r') as file:
            return yaml.safe_load(file)
    except FileNotFoundError:
        print(f"Error: The file '{file_path}' does not exist.")
        sys.exit(1)
    except yaml.YAMLError as exc:
        print(f"Error: Failed to parse YAML file '{file_path}'.\n{exc}")
        sys.exit(1)

# Plot component data against grid points and save the plot
def plot_component(grid, component_data, component_name, data_type, plot_dir):
    """Plot a component's data against grid points and save the plot."""
    plt.figure()
    plt.plot(grid, component_data, label=component_name)
    plt.xlabel('Grid Points')
    plt.ylabel(component_name)
    plt.title(f'{component_name} {data_type} vs Grid Points')  # Include data type
    plt.legend()

    plot_filename = os.path.join(plot_dir, f'{component_name}_{data_type}_vs_grid.png')
    plt.savefig(plot_filename)
    plt.close()

def main():
    if len(sys.argv) < 2:
        print("Usage: python script.py <path_to_yaml_file>")
        sys.exit(1)

    file_path = sys.argv[1]
    data = load_yaml_data(file_path)

    all_data = {}
    for key, value in data.items():
        match = re.match(r'(solution|residual)_(\d+)_(.+)', key)
        if match:
            data_type, solution_num, stage = match.groups()
            solution_num = int(solution_num)
            if solution_num not in all_data:
                all_data[solution_num] = {}
            if stage not in all_data[solution_num]:
                all_data[solution_num][stage] = {}
            all_data[solution_num][stage][data_type] = value['flame']

    for solution_num, stages in all_data.items():
        print(f'Plotting data for solution number: {solution_num}')

        for stage, data_types in stages.items():
            for data_type, oneD_data in data_types.items():
                components = oneD_data.get('components')
                grid = oneD_data.get('grid')

                if not components or not grid:
                    print(f"Warning: Missing 'components' or 'grid' data for solution {solution_num}, stage {stage}, type {data_type}. Skipping.")
                    continue

                # Separate directories for "solution" and "residual"
                plot_dir = os.path.join('debug_plots', f'solution_{solution_num}', stage, data_type)
                os.makedirs(plot_dir, exist_ok=True)

                for component in components:
                    if component in oneD_data and component != 'grid':
                        plot_component(grid, oneD_data[component], component, data_type, plot_dir)

                print(f"Plots for solution {solution_num}, stage {stage}, type {data_type} saved in directory: {plot_dir}")

if __name__ == "__main__":
    main()

Copy link
Copy Markdown
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Solver changes should be separated from formatting/docstring updates.

@wandadars
Copy link
Copy Markdown
Contributor Author

Solver changes should be separated from formatting/docstring updates.

I'll pull these changes apart into separate ones today.

@wandadars wandadars force-pushed the oneD_additions branch 2 times, most recently from faf73f4 to 2ec1a7d Compare August 5, 2024 17:51
@wandadars
Copy link
Copy Markdown
Contributor Author

Ok. I think I got them separated properly.

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.23%. Comparing base (4c8f9e9) to head (01c5393).
Report is 11 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@ischoegl
Copy link
Copy Markdown
Member

ischoegl commented Aug 5, 2024

Ok. I think I got them separated properly.

Thank you - this is much appreciated!

@ischoegl ischoegl dismissed their stale review August 6, 2024 13:33

Recommendation was accepted.

Copy link
Copy Markdown
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

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.

@wandadars
Copy link
Copy Markdown
Contributor Author

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?

@wandadars
Copy link
Copy Markdown
Contributor Author

I added some notes to that nonlinear solver section too @speth .

@speth
Copy link
Copy Markdown
Member

speth commented Aug 8, 2024

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 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 dTdz() method to get the temperature derivative, which does the upwinding based on the velocity. The implementation in #1079 was done to be consistent with the existing discretization. So, this may be worth revisiting, though I would recommend doing so in a standalone PR.

@wandadars wandadars changed the title OneD additions [OneD] OneD Documentation improvements[OneD] Aug 8, 2024
@wandadars
Copy link
Copy Markdown
Contributor Author

What's the best way to preview the markdown pages?

@ischoegl
Copy link
Copy Markdown
Member

ischoegl commented Aug 8, 2024

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 scons doxygen and scons sphinx locally if you have all the dependencies in place.

@wandadars
Copy link
Copy Markdown
Contributor Author

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 scons doxygen and scons sphinx locally if you have all the dependencies in place.

Thanks @ischoegl ! Man, that Sphinx dependency list is quite lengthy haha. Is the list of sphinx dependencies documented anywhere. For reference, it was:

sphinx
sphinx-argparse
sphinxcontrib-doxylink
sphinxcontrib-bibtex
myst-nb
sphinx-tags
sphinx-design
sphinx-copybutton
pydata-sphinx-theme
graphviz
pint
coolprop

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.

@ischoegl
Copy link
Copy Markdown
Member

ischoegl commented Aug 8, 2024

Thanks @ischoegl ! Man, that Sphinx dependency list is quite lengthy haha. Is the list of sphinx dependencies documented anywhere. For reference, it was:

sphinx
sphinx-argparse
sphinxcontrib-doxylink
sphinxcontrib-bibtex
myst-nb
sphinx-tags
sphinx-design
sphinx-copybutton
pydata-sphinx-theme
graphviz
pint
coolprop

Sure thing ... fwiw, here are conda environment recommendations, together with some instruction that @speth put together for the new documentation.

@wandadars
Copy link
Copy Markdown
Contributor Author

Ok, I think the notes are in a state where they can be examined.

Copy link
Copy Markdown
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, @wandadars, this is very good step toward having a well-documented 1D solver. I had a number of suggestions for further improvements.

Copy link
Copy Markdown
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, @wandadars. I think this is getting pretty close. Just some minor comments that should be easy to address.

Comment on lines +110 to +174
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)
$$
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Likewise, this misses the burner stabilized case, where the equation is that $\rho_j u_j = \dot{m}_{in}$.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@speth speth Aug 16, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, @wandadars. This looks good to me.

@speth speth merged commit 06261ed into Cantera:main Aug 18, 2024
@wandadars wandadars deleted the oneD_additions branch August 22, 2024 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants