Skip to content

Deprecate magic numbers in zeroD CLIB#660

Merged
speth merged 2 commits into
Cantera:masterfrom
ischoegl:deprecate-magic-numbers
Aug 1, 2019
Merged

Deprecate magic numbers in zeroD CLIB#660
speth merged 2 commits into
Cantera:masterfrom
ischoegl:deprecate-magic-numbers

Conversation

@ischoegl
Copy link
Copy Markdown
Member

@ischoegl ischoegl commented Jun 26, 2019

Please fill in the issue number this pull request is fixing:

Fixes #638

Changes proposed in this pull request:

  • add deprecation warning for int ReactorBase::type() (to be changed after Cantera 2.5)
  • introduce temporary std::string ReactorBase::typeStr() (to be renamed after Cantera 2.5)
  • deprecate all functions using the old call and introduce associated temporary functions

Temporary functions are necessary as overloading by return argument type is not possible in C++ (same for overloading of C functions).

PS: Updates are threaded through MATLAB, although I do not have a way to test at the moment (I am unsure whether unit tests are implemented) Edit: MATLAB testing is included in this PR.

@ischoegl ischoegl force-pushed the deprecate-magic-numbers branch 3 times, most recently from ed3c1e2 to ea3cbf4 Compare June 26, 2019 21:31
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 26, 2019

Codecov Report

Merging #660 into master will decrease coverage by 0.1%.
The diff coverage is 23.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #660      +/-   ##
==========================================
- Coverage   70.78%   70.67%   -0.11%     
==========================================
  Files         373      373              
  Lines       43447    43512      +65     
==========================================
  Hits        30754    30754              
- Misses      12693    12758      +65
Impacted Files Coverage Δ
include/cantera/zeroD/ReactorFactory.h 100% <ø> (ø) ⬆️
include/cantera/zeroD/WallFactory.h 64.28% <ø> (ø) ⬆️
include/cantera/zeroD/FlowDeviceFactory.h 64.28% <ø> (ø) ⬆️
include/cantera/zeroD/Wall.h 50% <ø> (ø) ⬆️
include/cantera/zeroD/ReactorBase.h 60.97% <0%> (-10.46%) ⬇️
include/cantera/zeroD/flowControllers.h 61.9% <0%> (-24.77%) ⬇️
include/cantera/zeroD/Reservoir.h 22.22% <0%> (-17.78%) ⬇️
include/cantera/zeroD/FlowDevice.h 47.82% <0%> (-16.88%) ⬇️
src/zeroD/ReactorNet.cpp 83.68% <100%> (ø) ⬆️
test_problems/clib_test/clib_test.c 100% <100%> (ø) ⬆️
... and 7 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 ae792dd...ea3cbf4. Read the comment docs.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 26, 2019

Codecov Report

Merging #660 into master will decrease coverage by 0.11%.
The diff coverage is 24.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #660      +/-   ##
==========================================
- Coverage   70.78%   70.66%   -0.12%     
==========================================
  Files         372      372              
  Lines       43443    43507      +64     
==========================================
- Hits        30750    30744       -6     
- Misses      12693    12763      +70
Impacted Files Coverage Δ
include/cantera/zeroD/ReactorFactory.h 100% <ø> (ø) ⬆️
include/cantera/zeroD/WallFactory.h 64.28% <ø> (ø) ⬆️
include/cantera/zeroD/FlowDeviceFactory.h 64.28% <ø> (ø) ⬆️
include/cantera/zeroD/Wall.h 50% <ø> (ø) ⬆️
include/cantera/zeroD/ReactorBase.h 55.55% <0%> (-15.88%) ⬇️
include/cantera/zeroD/Reservoir.h 22.22% <0%> (-17.78%) ⬇️
include/cantera/zeroD/FlowDevice.h 47.82% <0%> (-16.88%) ⬇️
test_problems/clib_test/clib_test.c 100% <100%> (ø) ⬆️
src/zeroD/ReactorNet.cpp 83.68% <100%> (ø) ⬆️
include/cantera/zeroD/flowControllers.h 71.42% <33.33%> (-15.24%) ⬇️
... and 9 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 296e291...5ed11a4. Read the comment docs.

@ischoegl ischoegl force-pushed the deprecate-magic-numbers branch 4 times, most recently from a381b35 to ccdf47a Compare June 26, 2019 22:33
@ischoegl ischoegl force-pushed the deprecate-magic-numbers branch 2 times, most recently from 49ef505 to 93aa49f Compare June 28, 2019 13:33
Comment thread include/cantera/zeroD/IdealGasReactor.h Outdated
Comment thread src/matlab/reactormethods.cpp Outdated
Comment thread src/matlab/flowdevicemethods.cpp Outdated
Comment thread include/cantera/zeroD/ReactorBase.h Outdated
Comment thread include/cantera/zeroD/ReactorBase.h Outdated
@ischoegl ischoegl force-pushed the deprecate-magic-numbers branch 2 times, most recently from b4a750b to fafc3f7 Compare July 1, 2019 14:17
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Jul 1, 2019

@speth ... I believe comments are addressed.

As mentioned earlier, I currently don't have a way to check matlab (I don't have a working toolchain at the moment).

@ischoegl ischoegl force-pushed the deprecate-magic-numbers branch from fafc3f7 to e219352 Compare July 2, 2019 03:50
Comment thread src/matlab/wallmethods.cpp Outdated
@ischoegl ischoegl force-pushed the deprecate-magic-numbers branch 2 times, most recently from f554df9 to e7a759f Compare July 2, 2019 22:17
@ischoegl ischoegl force-pushed the deprecate-magic-numbers branch 3 times, most recently from e485bb2 to e79bea3 Compare July 16, 2019 17:19
@ischoegl
Copy link
Copy Markdown
Member Author

WIP ... at this point, I finally have a working toolchain for matlab. I will report once the remaining issues are addressed.

@ischoegl ischoegl force-pushed the deprecate-magic-numbers branch 4 times, most recently from c8ed842 to d6b4efa Compare July 19, 2019 22:08
@ischoegl
Copy link
Copy Markdown
Member Author

@speth ... Matlab testing is now complete: the example script test_examples.m now runs without errors (tested for R2019).

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 think the only decision to be made here is on the Matlab version requirement. If changing this to support Matlab < 2016b is easy, then I would say we should do that. If it's annoying to refactor to do so, I don't really have a problem with setting that as the version requirement.

Comment thread interfaces/matlab/toolbox/@Reactor/Reactor.m Outdated
Comment thread src/matlab/flowdevicemethods.cpp Outdated
Comment thread src/matlab/flowdevicemethods.cpp Outdated
@ischoegl ischoegl force-pushed the deprecate-magic-numbers branch 2 times, most recently from 6d36964 to c39279d Compare July 31, 2019 21:06
 * add deprecation warning for int ReactorBase::type() (to be changed after Cantera 2.5)
 * introduce temporary std::string ReactorBase::typeStr() (to be renamed after Cantera 2.5)
 * deprecate all functions using the old call and introduce associated temporary functions
@ischoegl ischoegl force-pushed the deprecate-magic-numbers branch from c39279d to 5ed11a4 Compare July 31, 2019 21:09
@ischoegl
Copy link
Copy Markdown
Member Author

@speth ... thank you for the review. Replacing strings in Matlab was actually quite straight-forward (cell arrays of char arrays), and it's back to being compatible beyond 2016b.

@speth speth merged commit d263566 into Cantera:master Aug 1, 2019
@speth
Copy link
Copy Markdown
Member

speth commented Aug 5, 2019

@ischoegl The reason your commits aren't being properly attributed is because the "author" field is being filled in using your GMail address, presumably based on your Git configuration on one of your machines. You can get Github to link these commits to your account by adding that address as an alternate address for your Github account.

@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Aug 5, 2019

@speth ... this leaves me stumped: I have my primary email set to my gmail and backup set to the lsu domain (I added the latter recently). I just looked at my commit and it shows my gmail (!). I.e. I already have both my e-mails associated with the account.

@ischoegl ischoegl deleted the deprecate-magic-numbers branch October 5, 2019 00:11
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.

Deprecate magic numbers in zeroD CLIB interface

2 participants