Remove vestigial onedim Python functions and methods #1464

ischoegl · 2023-03-25T16:18:48Z

Changes proposed in this pull request

After moving SolutionArray support to C++ in #1426 and updating HDF support in #1385, remaining Python versions are replaced, vestigial functions are deprecated, and code is removed where appropriate. Some minor glitches are fixed.

Make Sim1D data vector accessible to Domain1D
Implement SolutionArray::normalize to normalize mass/mole fractions
Implement ThermoPhase::nativeMode to produce acronym describing storage mode
Fix minor bugs in new C++ SolutionArray interface
Rename some Python / C++ methods for consistency across interfaces
Use C++ SolutionArray methods for the interface between Python Sim1D and SolutionArray
Retain ability to restore legacy HDF using h5py for Cantera 3.0, but deprecate all associated methods
Remove OrderedDict, which is no longer necessary
Convert Python FutureWarnings to DeprecationWarnings
Replace some hardcoded indices
Deprecate h5py support

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

Resolves Cantera/enhancements#166

If applicable, provide an example illustrating new features this pull request is introducing

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

codecov · 2023-03-25T20:59:37Z

Codecov Report

Merging #1464 (572ea0e) into main (8b5a0e9) will increase coverage by 0.02%.
The diff coverage is 73.36%.

❗ Current head 572ea0e differs from pull request most recent head a412b61. Consider uploading reports for the commit a412b61 to get more accurate results

@@            Coverage Diff             @@
##             main    #1464      +/-   ##
==========================================
+ Coverage   69.86%   69.89%   +0.02%     
==========================================
  Files         377      377              
  Lines       57298    57350      +52     
  Branches    19164    19203      +39     
==========================================
+ Hits        40033    40085      +52     
- Misses      14712    14715       +3     
+ Partials     2553     2550       -3

Impacted Files	Coverage Δ
include/cantera/base/SolutionArray.h	`94.44% <ø> (ø)`
include/cantera/oneD/OneDim.h	`53.62% <ø> (ø)`
include/cantera/oneD/StFlow.h	`100.00% <ø> (ø)`
include/cantera/thermo/Phase.h	`78.57% <ø> (ø)`
interfaces/cython/cantera/solutionbase.pyx	`89.63% <0.00%> (ø)`
interfaces/cython/cantera/thermo.pyx	`92.23% <0.00%> (-0.13%)`	⬇️
src/oneD/Boundary1D.cpp	`55.39% <40.00%> (ø)`
include/cantera/oneD/Sim1D.h	`66.66% <50.00%> (ø)`
src/base/SolutionArray.cpp	`76.16% <55.55%> (-0.60%)`	⬇️
include/cantera/oneD/Domain1D.h	`79.69% <60.00%> (+0.46%)`	⬆️
... and 10 more

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Prevent automatic alphabetic ordering of states in SolutionArray::componentNames

Rename Domain1D::restore to Domain1D::fromArray Introduce alternative Domain1D::asArray/fromArray versions

Deprecate obsolete Python route and associated helper functions.

As of Python 3.7, dictionaries preserve order of insertion

ischoegl · 2023-03-25T23:55:10Z

@speth ... while #1462 is mostly about cosmetics, this PR addresses some hanging chads left after #1385 and #1426. Changes are ready for a review.

ischoegl · 2023-03-26T13:39:00Z

I added commits that replace hardcoded indices in StFlow. With these changes, tests show that changing the storage order now works for axisymmetric cases, but not for free flames. As locating the remaining hard-coded bits (presumably someplace around the fixed point) is beyond the scope of this PR, I am stopping here.

While the h5py deprecation warning needs improvement (there is a replacement!), I don’t want to add another force-push 😜 .

speth

Thanks, @ischoegl, I think this is generally fine. I just had some minor suggestions/comments.

speth · 2023-04-08T15:08:31Z

include/cantera/oneD/Domain1D.h

+    shared_ptr<vector<double>> m_data; //!< data pointer shared from OneDim
+


I'd like to find a more descriptive name for this array than just "data" (and likewise the setData method). Perhaps m_state, given that it's replacing m_x at the Sim1D level?

Also, we normally use the vector_fp typedef instead of writing vector<double>. This shows up several times in this PR. Unless you're suggesting preferring this name more broadly?

I renamed m_data to m_state as suggested.

Regarding vector_fp vs vector<double>, I do indeed have a preference for the latter, as it is self-documenting while the former is somewhat hidden in ct_defs.h. I believe the extra 5 characters are worthwhile, especially now that we no longer require std::.

include/cantera/thermo/Phase.h

interfaces/cython/cantera/_onedim.pyx

interfaces/cython/cantera/composite.py

interfaces/cython/cantera/_onedim.pyx

interfaces/cython/cantera/onedim.py

src/oneD/Domain1D.cpp

test/general/test_composite.cpp

test/python/test_composite.py

ischoegl · 2023-04-08T21:21:59Z

@speth ... thanks for the review! I believe I addressed most of the comments, with a remaining comment about vector_fp.

speth

Looks good to me!

ischoegl added the Python label Mar 25, 2023

ischoegl force-pushed the use-shared-onedim-data branch from 29ff2a0 to 9c015b6 Compare March 25, 2023 16:55

ischoegl mentioned this pull request Mar 25, 2023

Create direct data access for Domain1D objects Cantera/enhancements#167

Open

ischoegl force-pushed the use-shared-onedim-data branch from 9c015b6 to de2bb92 Compare March 25, 2023 17:19

ischoegl mentioned this pull request Mar 25, 2023

Solutionarray summary #1462

Merged

5 tasks

ischoegl force-pushed the use-shared-onedim-data branch 2 times, most recently from ae9562d to 5e1a630 Compare March 25, 2023 20:26

ischoegl force-pushed the use-shared-onedim-data branch from 5e1a630 to 2d1621f Compare March 25, 2023 21:46

ischoegl added 2 commits March 25, 2023 16:53

[Thermo] Add Phase::nativeMode / fix glitch

38f5f5b

Prevent automatic alphabetic ordering of states in SolutionArray::componentNames

[Python] Add ThermoPhase._native_mode

738223b

ischoegl force-pushed the use-shared-onedim-data branch 2 times, most recently from f651e49 to 1a931a6 Compare March 25, 2023 23:25

ischoegl added 13 commits March 25, 2023 18:25

[SolutionArray] Implement SolutionArray::normalize

27a5ab8

[unittest] Add gtest for SolutionArray::normalize

4069f4e

[oneD] Replace Sim1D::m_x by OneDim::m_data

e487285

[oneD] Share data vectors as Domain1D::m_data

c2a4f96

[oneD] Update Domain1D/SolutionArray interface

eaa36b0

Rename Domain1D::restore to Domain1D::fromArray Introduce alternative Domain1D::asArray/fromArray versions

[Python] Update FlameBase/SolutionArray interfaces

166f33f

Deprecate obsolete Python route and associated helper functions.

[Python] Deprecate obsolete 1D 'settings' properties

b90a24d

[Python] Deprecate unused FlameBase.other_components

917a45a

[Python] Convert FutureWarnings to DeprecationWarnings

f3a6925

[Python] Deprecate FlameBase.solution

4fdd400

[Python] Prevent python_to_anyvalue conversion failures

3da4c77

[Python] Remove OrderedDict

7541fdd

As of Python 3.7, dictionaries preserve order of insertion

[Python] Deprecate h5py support

b3b3988

ischoegl force-pushed the use-shared-onedim-data branch from 1a931a6 to e03b8ee Compare March 25, 2023 23:25

ischoegl marked this pull request as ready for review March 25, 2023 23:40

ischoegl requested a review from a team March 25, 2023 23:53

ischoegl force-pushed the use-shared-onedim-data branch 3 times, most recently from 1d040f4 to 572ea0e Compare March 26, 2023 13:31

speth requested changes Apr 8, 2023

View reviewed changes

ischoegl added 5 commits April 8, 2023 14:10

[oneD] Remove hardcoded component indices

065e581

[oneD] Remove hardcoded solution bounds

c522fc6

Remove h5py dependence

0349978

Improve nomenclature and docstrings

2f48d7f

[unittests] Update unittests

a412b61

ischoegl force-pushed the use-shared-onedim-data branch from 572ea0e to a412b61 Compare April 8, 2023 19:15

ischoegl requested a review from speth April 8, 2023 21:20

speth approved these changes Apr 9, 2023

View reviewed changes

speth merged commit f493f5b into Cantera:main Apr 9, 2023

ischoegl deleted the use-shared-onedim-data branch April 9, 2023 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove vestigial onedim Python functions and methods #1464

Remove vestigial onedim Python functions and methods #1464

ischoegl commented Mar 25, 2023 •

edited

Loading

codecov bot commented Mar 25, 2023 •

edited

Loading

ischoegl commented Mar 25, 2023

ischoegl commented Mar 26, 2023 •

edited

Loading

speth left a comment

speth Apr 8, 2023

ischoegl Apr 8, 2023 •

edited

Loading

ischoegl commented Apr 8, 2023

speth left a comment

		shared_ptr<vector<double>> m_data; //!< data pointer shared from OneDim

Remove vestigial onedim Python functions and methods #1464

Remove vestigial onedim Python functions and methods #1464

Conversation

ischoegl commented Mar 25, 2023 • edited Loading

codecov bot commented Mar 25, 2023 • edited Loading

Codecov Report

ischoegl commented Mar 25, 2023

ischoegl commented Mar 26, 2023 • edited Loading

speth left a comment

Choose a reason for hiding this comment

speth Apr 8, 2023

Choose a reason for hiding this comment

ischoegl Apr 8, 2023 • edited Loading

Choose a reason for hiding this comment

ischoegl commented Apr 8, 2023

speth left a comment

Choose a reason for hiding this comment

ischoegl commented Mar 25, 2023 •

edited

Loading

codecov bot commented Mar 25, 2023 •

edited

Loading

ischoegl commented Mar 26, 2023 •

edited

Loading

ischoegl Apr 8, 2023 •

edited

Loading