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

Fix external cantera cimport #1470

Merged
merged 2 commits into from
Apr 9, 2023
Merged

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Mar 30, 2023

Fix broken __init__.pxd file, which is required for the compilation of external Cython code. The file refers to _cantera.pxd, which was removed / split into multiple files in #1334. This PR adds all individual *.pxd files Cantera provides in its interface.

With this fix, external Cython modules can again access content via

cimport cantera as ct

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

Closes #1469

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

@ischoegl ischoegl requested a review from a team March 30, 2023 17:11
@ischoegl ischoegl force-pushed the fix-external-cython branch 3 times, most recently from 2ada466 to 4430de5 Compare March 30, 2023 17:18
@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Merging #1470 (4c04d86) into main (8b5a0e9) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1470      +/-   ##
==========================================
+ Coverage   69.86%   69.88%   +0.01%     
==========================================
  Files         377      377              
  Lines       57298    57298              
  Branches    19164    19164              
==========================================
+ Hits        40033    40040       +7     
+ Misses      14712    14705       -7     
  Partials     2553     2553              
Impacted Files Coverage Δ
interfaces/cython/cantera/constants.pyx 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

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

@ischoegl ischoegl marked this pull request as draft March 30, 2023 17:48
@ischoegl ischoegl marked this pull request as ready for review March 30, 2023 18:45
@ischoegl
Copy link
Member Author

Realized that there was another missing pxd file. This is ready for a review.

@ischoegl
Copy link
Member Author

Fwiw, I successfully tested the external interface here on macOS and Linux.

Comment on lines +7 to +10
cdef extern from "cantera/base/ct_defs.h" namespace "Cantera":
cdef double CxxAvogadro "Cantera::Avogadro"
cdef double CxxGasConstant "Cantera::GasConstant"
cdef double CxxOneAtm "Cantera::OneAtm"
Copy link
Member

Choose a reason for hiding this comment

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

Do these need to be exposed as the names CxxOneAtm etc? They should also available using the Python names one_atm etc if you use both import cantera as ct as well as cimport cantera as ct.

Copy link
Member Author

Choose a reason for hiding this comment

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

I’m less concerned about CxxOneAtm than some of the more fundamental constants, where a cimport is imho the cleaner route. Overall, I mainly added the constants as the missing file was an outlier in the interface.

@speth speth merged commit 839048d into Cantera:main Apr 9, 2023
@ischoegl ischoegl deleted the fix-external-cython branch April 9, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Broken interface for external Cython modules
2 participants