Add Solution to 1D boundaries

[ischoegl]

Changes proposed in this pull request

This PR is a follow-up to #1345 and adds Solution objects to all Boundary1D specializations instantiated from the Python API.

• ReactingSurf1D receives interface (instead of gas)
• Other Boundary1D objects receive gas phase adjacent to boundary (no change)
• Deprecate support for non-keyword phase arguments in Domain1D interface

If applicable, fill in the issue number this pull request is fixing

Preliminary work for Cantera/enhancements#137

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

[codecov]

Codecov Report

Merging #1384 (e3c54bd) into main (f6744ec) will increase coverage by 0.00%.
The diff coverage is 79.62%.

❗ Current head e3c54bd differs from pull request most recent head 75bc434. Consider uploading reports for the commit 75bc434 to get more accurate results

@@           Coverage Diff           @@
##             main    #1384   +/-   ##
=======================================
  Coverage   71.03%   71.03%           
=======================================
  Files         363      363           
  Lines       51855    51898   +43     
  Branches    17362    17366    +4     
=======================================
+ Hits        36834    36868   +34     
- Misses      12676    12683    +7     
- Partials     2345     2347    +2     

Impacted Files	Coverage Δ
include/cantera/oneD/Boundary1D.h	58.57% <69.23%> (+2.43%)	⬆️
src/oneD/Boundary1D.cpp	54.40% <71.42%> (+0.55%)	⬆️
interfaces/cython/cantera/_onedim.pyx	79.40% <88.46%> (+0.42%)	⬆️
interfaces/cython/cantera/onedim.py	88.22% <100.00%> (-0.02%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[bryanwweber]

Deprecate support for non-keyword phase arguments in Domain1D interface

I'm curious about the motivation for this change. I didn't see it discussed elsewhere, although I may have missed it 🤪

[ischoegl]

Deprecate support for non-keyword phase arguments in Domain1D interface

I'm curious about the motivation for this change. I didn't see it discussed elsewhere, although I may have missed it 🤪

... you didn't miss anything, but the old implementation was imho error-prone.

The Python 1D API uses *args, **kwargs extensively, effectively allowing to pass phase as either. One of the super constructors does eventually require phase to be a positional argument, which means that there are some lurking issues: Boundary1D.gas (which is rarely used) may not hold the expected object. I believe having a well-defined interface is ultimately the better option here.

[bryanwweber]

Boundary1D.gas (which is rarely used) may not hold the expected object

If this is the problem to fix, then enforcing phase to always be a kwarg is a pretty intrusive change, IMO. As it's always required it has the effect of increasing verbosity and restricting how the function can be called for the default use case of most of the API (although not the main user facing classes, FreeFlame etc. fortunately). It may as well be positional-only, to have the same effect of enforcing a standard API, no? That's not necessarily a serious suggestion, just to point out that I think it's not a clear win to make this kwarg-only. But this is deliberately a comment not a request-for-change review because in the end it probably doesn't make much of a difference whatever is chosen here.

[speth]

One of the super constructors does eventually require phase to be a positional argument, which means that there are some lurking issues: Boundary1D.gas (which is rarely used) may not hold the expected object.

Can you give an example of this? It isn't obvious to me. The signature of Domain1D.__init__ does require a phase object, but it's not required to be a positional argument:

cdef class Domain1D:
    ...
    def __init__(self, _SolutionBase phase, *args, name=None, **kwargs):

[ischoegl]

@speth ... I just removed the code portions in question (while making it impossible to pass phase as a non-positional argument), so the issue is moot. (I may have gotten lost with all the unspecific *args, **kwargs)

[ischoegl]

@bryanwweber ... based on your feedback, I reverted the changes. Instead, I made the Boundary1D interface much stricter - which is consistent with the existing test suite, while throwing more descriptive errors (which now also has a unit test).

[speth]

I get the impression that this is working toward something a bit larger -- can I ask what that might be, and how this contributes?

Adding Solution objects to the StFlow class in #1345 made sense, but I hadn't paid enough attention to the idea that it was also adding a Solution object to every Domain1D object. In particular, I think this ends up being a bit messy since for Boundary1D objects, there are in general two phases that matter: one associated with the bulk/flow domain to the left, and one to the right. Hence the m_phase_left and m_phase_right member variables of the Boundary1D class, which are automatically set when the containing OneDim object is initialized. I realize, however, that we don't currently provide an implementation of a Boundary1D that would actually sit between two flow domains, and this idea isn't supported at all in the Python interface.

The only case where a Boundary1D object has a phase object of its own that isn't already part of a neighboring domain is the ReactingSurf1D class. Other than that, the newly added Solution object seems to go unused, as far as I can tell.

[ischoegl]

I get the impression that this is working toward something a bit larger -- can I ask what that might be, and how this contributes?

See top of page: Preliminary work for Cantera/enhancements#137

I have a reasonable draft for an import of C++ solution arrays from YAML and HDF formats for Sim1D that are already part of 2.6 (which I can post). Solution objects are necessary to ensure that the states at the boundaries can be set properly. This is of particular importance for ReactingSurf1D.

Edit: I posted #1385 to provide context

[ischoegl]

To answer a little more in detail:

for Boundary1D objects, there are in general two phases that matter: one associated with the bulk/flow domain to the left, and one to the right. Hence the m_phase_left and m_phase_right member variables of the Boundary1D class, which are automatically set when the containing OneDim object is initialized. I realize, however, that we don't currently provide an implementation of a Boundary1D that would actually sit between two flow domains, and this idea isn't supported at all in the Python interface.

This is of course all true. At the same time, Python does provide a gas object for boundaries objects without specifying left or right (and requires a phase object when the boundary is created). All of them are external boundaries, so there is no ambiguity and doing the same for those objects in C++ would also make sense. As you probably saw I did make the surface phase the primary phase being added for ReactingSurf1D which I believe is the intuitive approach.

Beyond, I see boundary left/right in C++ as part of a more generic internal interface that was never fully implemented, as a dedicated internal boundary object is missing (as you noted). This PR is not trying to change any of that. At the moment, it is also impossible to replace the boundary left/right route as clib does not know about Solution.

[ischoegl]

@speth … any further comments?

[speth]

I think this is generally fine. Just had a few small questions / suggestions.

[ischoegl]

@speth ... thanks for the review! I believe I addressed everything.

@bryanwweber ... I hope you don't mind me cherry-picking your fix for the matplotlib warning from the units PR.

[speth]

Thanks, looks good to me!