-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Allow Cython module components to be compiled independently #1334
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1334 +/- ##
==========================================
- Coverage 68.04% 68.00% -0.04%
==========================================
Files 314 318 +4
Lines 42007 42007
Branches 16880 16880
==========================================
- Hits 28582 28568 -14
Misses 11167 11167
- Partials 2258 2272 +14
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
750a87a
to
231e839
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor changes here @speth for consistency/cleanup. Thanks for taking this on!
231e839
to
d5fb15a
Compare
d5fb15a
to
a2f8872
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Looks good to me, but it may be good to time the merge together with a resolution of #1258? Then merge conflicts in other PR’s can be resolved in a single pass.
I was thinking about tackling #1258 next, but I don't know that there's anything that particularly helps with the merge conflicts in branches/PRs that already exist. Having to resolve merge conflicts in more files at once doesn't really make things simpler. I think the most useful thing to do is merge this now (assuming there are no further comments) so that no new branches/PRs will be created that don't already incorporate these changes. |
@speth … good to hear that #1258 is next, so your suggestion is good. @bryanwweber … do you have anything else? |
@speth I added the license header to all the files in the Cython folder, I'm not sure if it was intentional to avoid adding it to the |
Looks good to me as well. @bryanwweber, please approve if you’re fine with everything |
I thought I had added the license header everywhere. Some of that must have gotten lost when I was fixing the merge conflicts to rebase past #1327 being merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @speth! In it goes!
Changes proposed in this pull request
.pyx
file so they can be compiled independentlyIf applicable, provide an example illustrating new features this pull request is introducing
This change substantially speeds up compilation of the Cython module when compiling with multiple cores, or when making changes that only affect a subset of the
.pyx
files.Timings for the current
main
branch:Timings for this PR:
Note that the total CPU time increases by a fair amount, due to the duplicated work done in compiling some common code for each
.pyx
file. However, even with 2 cores, the wall-clock time decreases.In addition, this provides some of the structure envisioned by Cantera/enhancements#153, in that the classes and methods are accessible using names qualified with the corresponding submodule, such as:
although they are also available directly in the
cantera
module as well. While I don't want to pursue this as part of the current PR, I think this restructuring also provides most of the necessary machinery to support building only a subset of the C++ source and the corresponding portions of the Python module, such as compiling everything except the reactor/flame models.Checklist
scons build
&scons test
) and unit tests address code coverage