[Thermo] Add property to call species charges in phase #863

korffdm · 2020-06-08T20:07:57Z

This commit adds the ability for the user to call the charge
of all species within a phase. This required updates to the
phase source code and the Python wrapper files.

Checklist

There is a clear use-case for this code change
The commit message has a short title & references relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
The pull request is ready for review

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

Fixes Cantera/enhancements#50
Similar to #862

Changes proposed in this pull request

Add ThermoPhase.species_charges as a Property in the Python wrapper
Follows format and method for molecular_weights code

Notes

In #862 there is already a test set up, is it possible to incorporate that test into this code or would a separate test need to be written for the approach used here?

ischoegl · 2020-06-08T20:15:26Z

In #862 there is already a test set up, is it possible to incorporate that test into this code or would a separate test need to be written for the approach used here?

@korffdm ... Feel free to recylce the test! Simply replace electrical_charges with species_charges and it should run. PR #862 is closed, so simply copy the section of test_thermo.py over to your PR.

codecov · 2020-06-08T20:30:27Z

Codecov Report

Merging #863 into master will decrease coverage by 0.01%.
The diff coverage is 30.00%.

@@            Coverage Diff             @@
##           master     #863      +/-   ##
==========================================
- Coverage   71.57%   71.56%   -0.02%     
==========================================
  Files         372      372              
  Lines       44501    44513      +12     
==========================================
+ Hits        31851    31854       +3     
- Misses      12650    12659       +9

Impacted Files	Coverage Δ
include/cantera/thermo/Phase.h	`86.04% <ø> (ø)`
src/clib/ct.cpp	`8.64% <0.00%> (-0.08%)`	⬇️
include/cantera/cython/wrappers.h	`83.87% <100.00%> (+0.26%)`	⬆️
src/thermo/Phase.cpp	`83.42% <100.00%> (+0.06%)`	⬆️
src/base/application.h	`75.00% <0.00%> (ø)`
include/cantera/base/xml.h	`100.00% <0.00%> (ø)`
src/equil/vcs_solve_TP.cpp	`68.47% <0.00%> (ø)`
include/cantera/base/AnyMap.h	`100.00% <0.00%> (ø)`
include/cantera/base/global.h	`93.33% <0.00%> (ø)`
include/cantera/oneD/StFlow.h	`96.26% <0.00%> (ø)`
... and 23 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 4b9adfa...32ec0f5. Read the comment docs.

This commit adds the ability for the user to call the charge of all species within a phase. This required updates to the phase source code and the Python wrapper files.

speth

Congrats on your first Cantera PR, @korffdm! I just have a few suggestions to improve this which I realized are mostly related to warts in the implementation of getMolecularWeights, which this parallels.

One question I have which others may want to weigh in on is whether the names used here should just be charges for the Python property and getCharges for the C++ method, rather than species_charges. There isn't anything else associated with a ThermoPhase that can have a charge, right?

include/cantera/thermo/Phase.h

src/matlab/phasemethods.cpp

include/cantera/thermo/Phase.h

src/clib/ct.cpp

Change requests were made for Phase.cpp, ct.cpp, and Phase.h. These were mostly simplification changes to streamline the code.

…lab toolbox

speth

Thanks for making these updates. I'd like to reiterate my earlier suggestion of removing the adjective "species" from the method/property names, since I think it should be clear that it's the species charges that are being returned.

include/cantera/thermo/Phase.h

src/thermo/Phase.cpp

interfaces/cython/cantera/thermo.pyx

interfaces/matlab/toolbox/@ThermoPhase/speciesCharges.m

Updated the property name to be just "charges" as well as simplifying the function 'getCharges' in Phase.cpp

bryanwweber

One small change here to the .m file, but otherwise looks good to me.

bryanwweber · 2020-06-19T19:44:38Z

interfaces/matlab/toolbox/@ThermoPhase/charges.m

+%     Vector of species charges. Units: elem. charge
+%
+
+ch = phase_get(tp.tp_id, 23);


You need a newline at the end of this file

Just an extra line below line 12? I added that and pushed, but I'm not seeing it on my github repo.

At least in my view (I think I'm using the new interface) it shows as a red circle with a line in it when there's no new line in the Files Changed view on the PR. The problem seems to be fixed by 32ec0f5 so it's all good. See here for a brief reasoning on why files should end with a blank line: https://unix.stackexchange.com/questions/18743/whats-the-point-in-adding-a-new-line-to-the-end-of-a-file

Change requests were made for Phase.cpp, ct.cpp, and Phase.h. These were mostly simplification changes to streamline the code.

[thermo] Add ThermoPhase.electrical_charges in Python

2f46918

[korffdm] added 2 commits June 8, 2020 16:16

[Thermo] Add property to call species charges in phase

554dea9

This commit adds the ability for the user to call the charge of all species within a phase. This required updates to the phase source code and the Python wrapper files.

[Test] Added test for species_charges in test_thermo.py.

46a479d

korffdm force-pushed the SpeciesCharge branch from 0bd79af to 46a479d Compare June 9, 2020 15:10

AUTHORS updated

c3a2bc5

speth requested changes Jun 9, 2020

View reviewed changes

[korffdm] added 2 commits June 11, 2020 11:05

Implemented change requests from PR Cantera#863

cdb2235

Change requests were made for Phase.cpp, ct.cpp, and Phase.h. These were mostly simplification changes to streamline the code.

[Interfaces] Added speciesCharges.m to access the property in the Mat…

57beed7

…lab toolbox

korffdm requested a review from speth June 18, 2020 21:11

speth requested changes Jun 19, 2020

View reviewed changes

include/cantera/thermo/Phase.h Outdated Show resolved Hide resolved

src/thermo/Phase.cpp Outdated Show resolved Hide resolved

interfaces/cython/cantera/thermo.pyx Outdated Show resolved Hide resolved

interfaces/matlab/toolbox/@ThermoPhase/speciesCharges.m Outdated Show resolved Hide resolved

[korffdm] added 2 commits June 19, 2020 12:10

[Thermo] changed property name "species_charges" to "charges"

5033276

Updated the property name to be just "charges" as well as simplifying the function 'getCharges' in Phase.cpp

Removing speciesCharges.m to match name change for property.

9a0e474

korffdm requested a review from speth June 19, 2020 18:20

bryanwweber approved these changes Jun 19, 2020

View reviewed changes

[Interfaces] Updated formatting on charges.m

32ec0f5

speth approved these changes Jun 19, 2020

View reviewed changes

speth merged commit 2736310 into Cantera:master Jun 19, 2020

speth pushed a commit that referenced this pull request Jun 19, 2020

Implemented change requests from PR #863

8306d08

Change requests were made for Phase.cpp, ct.cpp, and Phase.h. These were mostly simplification changes to streamline the code.

12Chao pushed a commit to 12Chao/cantera that referenced this pull request Jul 1, 2020

Implemented change requests from PR Cantera#863

9ad3d6f

Change requests were made for Phase.cpp, ct.cpp, and Phase.h. These were mostly simplification changes to streamline the code.

srikanthallu pushed a commit to srikanthallu/cantera that referenced this pull request Sep 17, 2020

Implemented change requests from PR Cantera#863

04e48ac

Change requests were made for Phase.cpp, ct.cpp, and Phase.h. These were mostly simplification changes to streamline the code.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Thermo] Add property to call species charges in phase #863

[Thermo] Add property to call species charges in phase #863

korffdm commented Jun 8, 2020 •

edited

Loading

ischoegl commented Jun 8, 2020 •

edited

Loading

codecov bot commented Jun 8, 2020 •

edited

Loading

speth left a comment

speth left a comment

bryanwweber left a comment

bryanwweber Jun 19, 2020

korffdm Jun 19, 2020

bryanwweber Jun 19, 2020 •

edited

Loading

[Thermo] Add property to call species charges in phase #863

[Thermo] Add property to call species charges in phase #863

Conversation

korffdm commented Jun 8, 2020 • edited Loading

ischoegl commented Jun 8, 2020 • edited Loading

codecov bot commented Jun 8, 2020 • edited Loading

Codecov Report

speth left a comment

Choose a reason for hiding this comment

speth left a comment

Choose a reason for hiding this comment

bryanwweber left a comment

Choose a reason for hiding this comment

bryanwweber Jun 19, 2020

Choose a reason for hiding this comment

korffdm Jun 19, 2020

Choose a reason for hiding this comment

bryanwweber Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

korffdm commented Jun 8, 2020 •

edited

Loading

ischoegl commented Jun 8, 2020 •

edited

Loading

codecov bot commented Jun 8, 2020 •

edited

Loading

bryanwweber Jun 19, 2020 •

edited

Loading