Skip to content

Standardize code cont'd#1568

Merged
speth merged 15 commits intoCantera:mainfrom
ischoegl:standardize-code-contd
Aug 5, 2023
Merged

Standardize code cont'd#1568
speth merged 15 commits intoCantera:mainfrom
ischoegl:standardize-code-contd

Conversation

@ischoegl
Copy link
Copy Markdown
Member

@ischoegl ischoegl commented Aug 3, 2023 •
edited
Loading

Changes proposed in this pull request

Implement remaining items from Cantera/enhancements#181

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

Closes Cantera/enhancements#181

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 force-pushed the standardize-code-contd branch 2 times, most recently from fa74984 to ecd010c Compare August 3, 2023 23:23
@ischoegl ischoegl mentioned this pull request Aug 3, 2023
Closed
15 tasks
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 4, 2023 •
edited
Loading

Codecov Report

Merging #1568 (0622402) into main (e0a2f11) will increase coverage by 0.03%.
The diff coverage is 82.70%.

@@            Coverage Diff             @@
##             main    #1568      +/-   ##
==========================================
+ Coverage   70.50%   70.53%   +0.03%     
==========================================
  Files         379      379              
  Lines       59112    59110       -2     
  Branches    21232    21232              
==========================================
+ Hits        41675    41695      +20     
+ Misses      14361    14340      -21     
+ Partials     3076     3075       -1
Files Changed Coverage Δ
include/cantera/base/ExtensionManager.h 27.27% <0.00%> (ø)
include/cantera/base/Interface.h 100.00% <ø> (ø)
include/cantera/base/NoExitLogger.h 0.00% <0.00%> (ø)
include/cantera/base/ValueCache.h 100.00% <ø> (ø)
include/cantera/base/YamlWriter.h 100.00% <ø> (ø)
include/cantera/equil/vcs_MultiPhaseEquil.h 100.00% <ø> (ø)
include/cantera/equil/vcs_SpeciesProperties.h 100.00% <ø> (ø)
include/cantera/kinetics/MultiRateBase.h 100.00% <ø> (ø)
include/cantera/kinetics/ReactionData.h 67.85% <0.00%> (ø)
include/cantera/kinetics/solveSP.h 100.00% <ø> (ø)
... and 258 more

... and 2 files 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 ready for review August 4, 2023 00:15
@ischoegl ischoegl requested a review from a team August 4, 2023 00:15
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Aug 4, 2023

@speth ... per request, I added all remaining 'standardizations' in a single PR. I did not touch include/cantera/numerics/ctlapack.h, which continues to use doublereal.

@ischoegl ischoegl removed the request for review from a team August 4, 2023 00:45
@ischoegl ischoegl marked this pull request as draft August 4, 2023 00:46
@ischoegl ischoegl force-pushed the standardize-code-contd branch from ecd010c to 0abe19c Compare August 4, 2023 00:58
@ischoegl ischoegl marked this pull request as ready for review August 4, 2023 01:42
@ischoegl ischoegl requested a review from a team August 4, 2023 01:43
@ischoegl ischoegl force-pushed the standardize-code-contd branch from 0abe19c to 74aff50 Compare August 4, 2023 23:06
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Aug 4, 2023

Rebased to resolve merge conflict after #1570

speth added 2 commits August 4, 2023 23:52
@speth
Fix Matlab toolbox compilation
6383c8b 
This code isn't part of namespace Cantera, so can't use the
typedefs that are specified in that namespace.
@speth
Clean up line wrapping and indentation
2c3af3f
speth
speth reviewed Aug 5, 2023
View reviewed changes
Copy link
Copy Markdown
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.

I added a couple of commits to:

  1. fix compilation of the Matlab toolbox, which defines some functions outside of the Cantera namespace and therefore doesn't inherit the using statements from that namespace.
  2. Do some opportunistic formatting cleanup where the shorter names allowed it. I figured just pushing this change was more efficient than leaving 100 comments on this PR.

Is there a reason for not specifically deprecating doublereal and updating ctlapack.h? I suspect the rationale described in config.h.in is a little bit misguided -- I don't think there's any way for for the LAPACK/Fortran double precision type to be anything other than double, and even if this was possible, I don't think it could be handled by a simple typedef.

@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Aug 5, 2023 •
edited
Loading

Thanks for pushing corrections/updates directly, @speth ... didn't think about Matlab not using the namespace / ct_defs.h. Regarding reformatting, I did a regex search for the most obvious changes, but apparently missed numerous spots. Probably I was too opportunistic ... .

Regarding leaving doublereal in ctlapack.h, I wasn't sure about the history, and decided to leave it (in part also because of the comment in config.h.in you mentioned). I'm not too concerned about doublereal surviving a little longer, as I'm sure it makes no difference to the compiler.

Unless you have other suggestions, this is good to go from my side. Let's see what merge conflicts will arise in #1567 (should be easy to clean up though).

@speth speth merged commit 3332319 into Cantera:main Aug 5, 2023
@ischoegl ischoegl deleted the standardize-code-contd branch August 5, 2023 20:29
This was referenced Aug 6, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@speth speth speth approved these changes

Assignees

No one assigned

Labels

None yet

Projects

No open projects
Cantera 3.0 Release Planning
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove unnecessary typedef's

2 participants

@ischoegl @speth