Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Python extension loading and other Python improvements #1456

Merged
merged 6 commits into from
Mar 19, 2023

Conversation

speth
Copy link
Member

@speth speth commented Mar 18, 2023

Changes proposed in this pull request

  • Simplify loading of Python extension support by letting Cython handle the complexity of loading the extension module after Python has already been initialized. In the case of a non-Python main program, all we have to do is initialize Python and import the Cantera Python module.
  • Add support for Cython 3.0.0b1
  • Convert C++ NotImplementedError to Python native NotImplementedError
  • Improve documentation of _SolutionBase._references

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

Let Cython handle the complexity of loading the extension module after
Python has already been initialized. In the case of a non-Python main
program, all we have to do is initialize Python and import the Cantera
Python module.
@speth speth added the Python label Mar 18, 2023
@codecov
Copy link

codecov bot commented Mar 18, 2023

Codecov Report

Merging #1456 (e2d6d23) into main (0c5fc1d) will increase coverage by 0.06%.
The diff coverage is 90.38%.

@@            Coverage Diff             @@
##             main    #1456      +/-   ##
==========================================
+ Coverage   69.86%   69.92%   +0.06%     
==========================================
  Files         373      377       +4     
  Lines       56684    57262     +578     
  Branches    18883    19151     +268     
==========================================
+ Hits        39600    40043     +443     
- Misses      14571    14669      +98     
- Partials     2513     2550      +37     
Impacted Files Coverage Δ
interfaces/cython/cantera/mixture.pyx 90.47% <ø> (ø)
interfaces/cython/cantera/reactor.pyx 90.54% <ø> (ø)
interfaces/cython/cantera/solutionbase.pyx 89.66% <ø> (ø)
include/cantera/cython/funcWrapper.h 70.83% <50.00%> (-0.36%) ⬇️
interfaces/cython/cantera/delegator.pyx 79.91% <89.74%> (+1.09%) ⬆️
interfaces/cython/cantera/_onedim.pyx 79.76% <100.00%> (+0.27%) ⬆️
interfaces/cython/cantera/composite.py 89.16% <100.00%> (+0.09%) ⬆️
interfaces/cython/cantera/func1.pyx 100.00% <100.00%> (ø)
interfaces/cython/cantera/thermo.pyx 92.35% <100.00%> (ø)

... and 26 files with indirect coverage changes

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

Copy link
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.

@speth ... this looks great overall. The only item I'd request would be a todo comment to move sentinels to C++ after Cantera 3.0.

@speth
Copy link
Member Author

speth commented Mar 18, 2023

I created an issue (#1457) to track the needed feature. I think that's preferable to a to-do item buried somewhere in the code.

Copy link
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.

I created an issue (#1457) to track the needed feature. I think that's preferable to a to-do item buried somewhere in the code.

Thanks for creating the issue! I agree that it is preferable to a todo.

It did, however, occur to me that SolutionArray needs a similar sentinel, somewhere around here:

def __init__(self, phase, shape=(0,), states=None, extra=None, meta={}, init=True):
self._phase = phase

Even prior to the introduction of C++ SolutionArray, adding species would have created inconsistencies in stored data.

PS: Another item I noticed is that we may want to test with Cython 3.0 again?

@speth
Copy link
Member Author

speth commented Mar 19, 2023

I've added the Python weakref-style checks to both SolutionArray and Quantity.

I've also added a new CI job that runs only after pushes to main, and tests whatever the current pre-release version of Cython is. This creates a home for some of the tests that we've discussed in Cantera/enhancements#37, where running them on every push to a PR is more trouble than it's worth. I tested it by pushing this branch directly to Cantera/cantera. You can see the status (green checkmark) for this job reported here: https://github.com/Cantera/cantera/tree/python-ext-init. I'll delete that branch after we merge this PR.

Copy link
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.

Thanks, @speth … everything looks good to me!

@ischoegl ischoegl merged commit 3cbe8aa into Cantera:main Mar 19, 2023
@speth speth deleted the python-ext-init branch March 19, 2023 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants