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

FEAT: Added additional tools for Fortran calling codes #1154

Merged
merged 3 commits into from
Jan 19, 2022

Conversation

tpg2114
Copy link

@tpg2114 tpg2114 commented Dec 6, 2021

This wraps a few additional functions into Fortran that we use in our calling code, and also adds a new feature -- an error logger that does not exit from within Cantera so a calling code can handle errors that arise. Mostly useful for Fortran calling codes that cannot catch exceptions.

@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #1154 (43c9f45) into main (7fc5a2d) will decrease coverage by 7.66%.
The diff coverage is 30.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1154      +/-   ##
==========================================
- Coverage   73.76%   66.09%   -7.67%     
==========================================
  Files         370      314      -56     
  Lines       48983    44980    -4003     
  Branches        0    19146   +19146     
==========================================
- Hits        36133    29731    -6402     
+ Misses      12850    12665     -185     
- Partials        0     2584    +2584     
Impacted Files Coverage Δ
include/cantera/base/NoExitLogger.h 0.00% <0.00%> (ø)
src/fortran/fct.cpp 17.93% <32.14%> (+1.11%) ⬆️
include/cantera/transport/MultiTransport.h 50.00% <0.00%> (-50.00%) ⬇️
include/cantera/base/Units.h 70.00% <0.00%> (-30.00%) ⬇️
include/cantera/base/AnyMap.inl.h 51.85% <0.00%> (-26.90%) ⬇️
include/cantera/kinetics/KineticsFactory.h 73.33% <0.00%> (-26.67%) ⬇️
include/cantera/base/ctexceptions.h 50.00% <0.00%> (-26.20%) ⬇️
src/numerics/ODE_integrators.cpp 50.00% <0.00%> (-25.00%) ⬇️
include/cantera/transport/TransportBase.h 27.02% <0.00%> (-21.59%) ⬇️
include/cantera/thermo/ThermoFactory.h 80.00% <0.00%> (-20.00%) ⬇️
... and 302 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fc5a2d...43c9f45. Read the comment docs.

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution. While I eventually expect to be able to generate much of this interface in an automated fashion (Cantera/enhancements#39), that's still in it's early stages, so updates to this interface as it exists now are welcome.

I mostly had a few formatting suggestions. The other thing that I think would be valuable is calling a few of these new functions from the existing Fortran 90 test program (samples/f90/demo.f90).

include/cantera/base/NoExitLogger.h Outdated Show resolved Hide resolved
include/cantera/base/NoExitLogger.h Outdated Show resolved Hide resolved
include/cantera/base/NoExitLogger.h Outdated Show resolved Hide resolved
src/fortran/cantera_funcs.f90 Show resolved Hide resolved
src/fortran/cantera_thermo.f90 Outdated Show resolved Hide resolved
src/fortran/cantera_thermo.f90 Outdated Show resolved Hide resolved
src/fortran/fct.cpp Outdated Show resolved Hide resolved
src/fortran/fct.cpp Outdated Show resolved Hide resolved
src/fortran/fct_interface.f90 Outdated Show resolved Hide resolved
Tim Gallagher added 2 commits December 31, 2021 15:20
The new logger does not exit on an error so the calling code can
handle errors itself.
Addressed points raised in the review on pull request. Added calls
to new routines in the sample F90 program and cleaned up a few of
the routine interfaces to remove deprecated/unused info.
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks for the updates -- the changes look good to me. I hadn't intended for you to have to update the style of any surrounding code, only what was new in this PR, but thanks for taking on a few of those updates anyway.

I just pushed an update to your branch to add a couple comments to the NoExitLogger.h header so that it will appear in the documentation and so that it has the usual license statement carried by all Cantera source files.

Two small things, which you can update at your discretion, would be (1) to add yourself to the AUTHORS file and (2) to update update the email address that's set for Git on your computer to one that is associated with your GitHub account so those commits will be attributed correctly. There are some hopefully helpful instructions here: https://stackoverflow.com/questions/3042437/how-to-change-the-commit-author-for-one-specific-commit.

@tpg2114
Copy link
Author

tpg2114 commented Dec 31, 2021

No problem on updates -- I ask the folks I develop with to always leave a file better than they left it, so bringing the whole file up to date is worth the couple of extra seconds it took to find-replace things. I think the tests that failed when I first pushed the latest updates were due to the Fortran test having additional output from the calls to the new functions, and I wasn't comfortable changing the blessed solution without somebody "official" checking it out first.

@speth
Copy link
Member

speth commented Dec 31, 2021

Right, I forgot about the changes to the blessed output file. Since the changes are pretty simple (just some additional output lines), I would suggest copying those extra lines from test_problems/fortran/f90_demo_output.txt to test_problems/fortran/f90_demo_blessed.txt, without editing any of the other lines (which may have changed in the last decimal places, below the test's threshold).

@tpg2114
Copy link
Author

tpg2114 commented Dec 31, 2021

Done and pushed! Hopefully the tests will all pass now.

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me.

@speth speth merged commit 80e9362 into Cantera:main Jan 19, 2022
@tpg2114 tpg2114 deleted the additional_fortran branch January 20, 2022 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants