Update parameter names in for ThermoPhase.equilibrate #684
Conversation
interfaces/cython/cantera/thermo.pyx
Outdated
| @@ -379,6 +379,22 @@ cdef class ThermoPhase(_SolutionBase): | |||
| :param loglevel: | |||
| Set to a value > 0 to write diagnostic output. | |||
| """ | |||
| # check for old parameters and warn the user | |||
| if 'maxsteps' in **kwargs: | |||
There was a problem hiding this comment.
Currently there is a syntax error here (which caused the CI checks to fail): when checking the contents of a dictionary, the ** that unpack it shouldn't be there. I.e.
if 'maxsteps' in kwargs:
...should work. Before resubmitting, I'd also recommend running scons build and scons test on your own machine, as it will catch those glitches.
|
@sameshl ... thanks for the PR. The wording of the warning may need some tweaks, but that goes above my pay grade ;) |
There was a problem hiding this comment.
As @ischoegl said, thank you for the changes here! I've also requested some additional changes.
interfaces/cython/cantera/thermo.pyx
Outdated
| log_level = log_level | ||
| warnings.warn("To be deprecated after Cantera 2.5." | ||
| "Use property `log_level` instead", DeprecationWarning) | ||
|
|
||
| self.thermo.equilibrate(stringify(XY.upper()), stringify(solver), rtol, | ||
| maxsteps, maxiter, estimate_equil, loglevel) |
There was a problem hiding this comment.
If you're changing the variable names, you have to actually use the new names in the function call here.
interfaces/cython/cantera/thermo.pyx
Outdated
| if 'loglevel' in **kwargs: | ||
| log_level = log_level | ||
| warnings.warn("To be deprecated after Cantera 2.5." | ||
| "Use property `log_level` instead", DeprecationWarning) |
There was a problem hiding this comment.
No need to include the backticks, this output will be shown unformatted anyways. I think the text of the warning message should say:
Keyword argument 'loglevel' is deprecated and will be removed after Cantera 2.5.0. Use argument 'log_level' instead.
And similar for the other warnings.
|
@bryanwweber Thanks for the review! Will make the required changes. |
|
@sameshl ... currently the build fails as you need to pick out Also, please squash your commits into a single commit using rebase ( |
|
@bryanwweber thanks a lot for offering help! 😅 But the thing is I was encountering a lot of issues setting up my dev env so I could not run build tests. I will try to rebase the commits. Will let you know if I have any problems |
Codecov Report
@@ Coverage Diff @@
## master #684 +/- ##
==========================================
+ Coverage 70.62% 70.62% +<.01%
==========================================
Files 372 372
Lines 43565 43565
==========================================
+ Hits 30768 30769 +1
+ Misses 12797 12796 -1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #684 +/- ##
==========================================
- Coverage 71.35% 71.34% -0.01%
==========================================
Files 372 372
Lines 43758 43758
==========================================
- Hits 31223 31221 -2
- Misses 12535 12537 +2
Continue to review full report at Codecov.
|
|
@bryanwweber Made the required changes! Let me know if I need to do anythink else! |
Changed keyword arguments from: * `maxsteps` to `max_steps` * `maxiter` to `max_iter` * `loglevel` to `log_level` Minor docstring formatting updates. closes #675
changed keyword arguments from :
maxstepstomax_stepsmaxitertomax_iterlogleveltolog_levelcloses #675