Fix examples #905
Fix examples #905
Conversation
bryanwweber
force-pushed
the
fix-examples
branch
2 times, most recently
from
0ac0d99
to
819a60e
Compare
Codecov Report
@@ Coverage Diff @@
## main #905 +/- ##
=======================================
Coverage 71.19% 71.19%
=======================================
Files 376 376
Lines 46211 46218 +7
=======================================
+ Hits 32898 32903 +5
- Misses 13313 13315 +2
Continue to review full report at Codecov.
|
|
@bryanwweber ... thanks for fixing. I'm happy to see running of examples added to CI. I'm still not sure why I got so much blow-back when I suggested the same in #892 (which I closed and moved to Cantera/enhancements#60 as it appeared that full regression tests were the preferred option). Doing the same from bash is probably less comprehensive than my PS: the one issue that came up with #892 are |
|
PPS: one other thing that came up in my own tests is poor convergence of |
|
Thanks for the comments @ischoegl! The concern that I had with your previous approach was exactly that it was adding commands to SCons (making that more complicated) without adding what we want to eventually do. As I said, adding the running here is in part to make sure that I've actually fixed what I set out to fix and avoid "It works on my machine!" problems. I would be fine with deleting that CI run before this PR is merged, once all the examples are fixed up, if we think it adds too much overhead. It will be as easy as deleting the lines from the workflow, without having to modify several of the SConstruct/SConscript files. It was an intentional choice not to try to make something that works on all the platforms, since my idea was to have the simplest possible thing to add and the simplest possible thing to remove when the time comes. The |
|
@bryanwweber ... you're welcome. I am strongly in favor of running examples during CI in one form or another. Ultimately it makes little difference whether this is done using your approach here or what I had put together in #892. Regarding |
bryanwweber
force-pushed
the
fix-examples
branch
7 times, most recently
from
9dfb7c2
to
5d7455a
Compare
|
The examples appear to be working now, with the exception of |
|
@bryanwweber ... I would advocate for a temporary removal of the |
|
|
|
@bryanwweber ... nevermind, I found a band-aid fix for |
|
Thanks, I saw that change. Does that have any other effect on the solution, are the boundary conditions satisfied, etc.? |
The boundary conditions - as implemented - are satisfied by the solver; the physicality is a different issue. Curiously, @BangShiuh removed the offending |
bryanwweber
force-pushed
the
fix-examples
branch
from
462441f
to
163dde6
Compare
|
@bryanwweber ... I saw that you cherry-picked the commits from #912 as discussed 👍. Beyond, I think #899 is ready to be merged as long as my updated comment is ok. |
interfaces/cython/cantera/examples/kinetics/extract_submechanism.py
Outdated
Show resolved
Hide resolved
|
@bryanwweber ... the conflict that just popped up is due to 7c42402 (where a smaller fix was just merged in #899). I didn't realize that I found issues twice with the same file. |
|
Thanks @ischoegl I'll sort out the conflict and amend the commit |
When loading reactions from a YAML file, an instance of a Kinetics object is required so that the species associated with the reactions are available. The required object is now constructed from the already loaded species definitions. Fixes Cantera#821, problem introduced in 1866e35
This change reworks the abortion criteria for the diffusion flame extinction example. Previously, the loop was only aborted when the flame temperature was above the inlet temperature, the difference of the flame temperature in the prior two iterations was less than the threshold, AND the change in strain rate parameter was less than the threshold. However, I found that this combination of conditions was never reached, because the only way to change the strain rate delta was to have an extinguished flame, and eventually the flame remains extinguished even if the delta_alpha is reduced. This leads to an infinite loop, since the abortion criteria check are only done if the flame is burning. This change moves the abortion criteria check into the case where the flame is not burning. In addition, the flame temperature check is modified to use np.isclose(). I found that on the last burning iteration, the maximum flame temperature was only just barely above the oxidizer inlet temperature. Thus, the result printed to the screen was not very useful. The change to np.isclose() produces a more useful result for the final burning iteration before extinction. The last non-burning iteration is also saved to the output file(s) for reference. Fixes Cantera#872
The set_boundary_emissivities method is deprecated and will be removed after Cantera 2.5.0. This change switches one example to use the new property. The property was also missing from the FlameBase class.
This allows the script to be run from the installed location, or from the root of the source tree.
argon.yaml is deprecated, use air.yaml and specify the composition instead.
The calculation for the production of gas phase species from surface reactions assumed that the gas phase species were stored at the beginning of the rate-of-production array. This is only the case when the gas ThermoPhase instance is the first phase in the Kinetics thermo vector, which is not guaranteed to be the case. It happens that XML input usually forced the gas phase to be the first phase in the thermo vector, so everything worked out fine. This was because the associated phase(s) for a surface phase had to be listed in an XML file, and the XML initializer reads that list to fill the Kinetics thermo vector. In the YAML format, the associated phase does not need to be explicitly listed. Instead, instances of the appropriate phases are passed to the constructor when the InterfaceKinetics is created. The gas phase can then come before or after the surface phase, meaning there is no guarantee of the order of species. This change checks for the starting index of the gas phase species in the InterfaceKinetics species-length arrays to make sure that the appropriate rates of production are added for the gas phase species. Fixes Cantera#902, Cantera#877, Cantera#873
Adds a getter function for the coverage_enabled property on the ReactingSurface1D Python class, and the corresponding C++ method.
The switch to the YAML input file revealed that the calculation of the anode and cathode currents in sofc.py relied on a particular order of the phases. This change corrects that by finding the index of the electron "species" in the anode and cathode phases.
Matplotlib issues a UserWarning if the xticklabels are explicitly set but the xticks are not explicitly set. We choose to set ticks every 360 degrees of crank angle, which corresponds to every 0.02 seconds in the simulation.
One of the Fortran samples needs to include the extra_inc_dirs variable so that system header files in non-standard locations will be found.
Use pathlib to write the text content to a file in write_dot. This automatically closes the file handle after the text is written. Also modify the reaction_path.py example to use pathlib and subprocess. These are available since at least Python 3.5 and represent modern best-practices in Python scripts.
The multiprocess.Pool object must be closed properly, or the process can hang according to the Python docs. The recommended method for short code blocks is to use the context manager. The format of the arguments to Pool is chosen to allow the last line to be de-dented to remove ambiguity about the definition of the context manager and the content.
pandas v1.1.0 issues a FutureWarning when plotting single columns from a DataFrame using the Matplotlib pyplot API. This can be avoided by using the pandas plotting API instead.
The content of ch4_ion.yaml now matches ch4_ion.cti (the file was re-created using cti2yaml)
A reduction of domain size makes the example more stable.
bryanwweber
force-pushed
the
fix-examples
branch
from
163dde6
to
42fbd1d
Compare
|
@speth Thanks for the comments, I believe I addressed all of them. The only other change I was considering was to switch how |
|
Thanks, that looks good to me. I'm not too concerned about that assumption -- I think you'd have a whole bunch of other problems if you re-used species names between phases in an |
Changes proposed in this pull request
find ... -exec pythonto run the examples and just double check that they all work. I added this because I expect that doing real regression testing in SCons will require quite a bit of effort and so will be some way off. This stop gap will be easy to delete or modify when the time comes. It is not intended to be comprehensive or something a developer can run on their own machine (although copying the command is effective). It is intended as a double check on my work here and a hopeful hedge against future breakage.Copies of commit messages for reference:
[Cython/Examples] Fix extract_submechanism example
The Reaction.listFromFile() method requires a Kinetics instance when
reading a YAML file. In this case, the appropriate Kinetics instance
could be created by Solution('gri30.yaml'). Since we'd have to create
that object anyways to be able to use Reaction.listFromFile(), we might
as well use the Solution.reactions() method, which returns the same
list as Reaction.listFromFile() for this case where the species
definitions are in the same file as the reaction definitions.
Fix infinite loop in diffusion_flame_extinction.py
This change reworks the abortion criteria for the diffusion flame
extinction example. Previously, the loop was only aborted when the flame
temperature was above the inlet temperature, the difference of the flame
temperature in the prior two iterations was less than the threshold, AND
the change in strain rate parameter was less than the threshold.
However, I found that this combination of conditions was never reached,
because the only way to change the strain rate delta was to have an
extinguished flame, and eventually the flame remains extinguished even
if the delta_alpha is reduced. This leads to an infinite loop, since the
abortion criteria check are only done if the flame is burning. This
change moves the abortion criteria check into the case where the flame
is not burning.
In addition, the flame temperature check is modified to use
np.isclose(). I found that on the last burning iteration, the maximum
flame temperature was only just barely above the oxidizer inlet
temperature. Thus, the result printed to the screen was not very useful.
The change to np.isclose() produces a more useful result for the final
burning iteration before extinction. The last non-burning iteration is
also saved to the output file(s) for reference.
Fix boundary emissivities deprecation warning
The set_boundary_emissivities method is deprecated and will be removed
after Cantera 2.5.0. This change switches one example to use the new
property. The property was also missing from the FlameBase class.
[Example] Specify data file path in flame_fixed_T
This allows the script to be run from the installed location, or from
the root of the source tree.
[Examples] Replace argon.yaml with air.yaml
argon.yaml is deprecated, use air.yaml and specify the composition
instead.
Remaining Fixes
Per #902, we need to determine a final solution for the loading of YAML files with surface mechanisms. I expect the new job to fail until I figure out how to fix that.
If applicable, fill in the issue number this pull request is fixing
Fixes #821, #902, #877, #873, #872
Checklist
scons build&scons test) and unit tests address code coverage