Implement Python dict to C++ AnyMap conversion by ischoegl · Pull Request #1014 · Cantera/cantera

ischoegl · 2021-04-17T15:25:04Z

Changes proposed in this pull request

This PR adds the ability to create C++ AnyMap and AnyValue objects from the Python interface, where dict_to_anymap/python_to_anyvalue mirrors recently introduced anymap_to_dict/anyvalue_to_python (see PR #984). The following uses are anticipated:

Instantiation of core objects from the Python API, e.g. reactions, see Make Python interface to reactions and reaction rates more intuitive enhancements#84
Serialization of --extra fields in YAML serialization, see Serialization of root-level information in YAML #1013
Creation of user-data attached to various Cantera objects, see Formalize YAML metadata fields enhancements#56

The new service functions enable the use of C++ constructors (or other C++ functions) that take AnyMap or AnyValue as an argument from the Cython layer. Round-trip conversion is covered by unit tests via a hidden helper function cantera._cantera._py_to_any_to_py in a new test_utils.py job. Beyond, the functions are not accessible from the Python API, as they are internal service functions that facilitate the transfer of data from Python to the C++ layer.

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

Also, add missing case for conversion of list of dictionaries

codecov · 2021-04-17T21:07:59Z

Codecov Report

Merging #1014 (87ad140) into main (81cffde) will increase coverage by 0.14%.
The diff coverage is 100.00%.

❗ Current head 87ad140 differs from pull request most recent head 32bf083. Consider uploading reports for the commit 32bf083 to get more accurate results

@@            Coverage Diff             @@
##             main    #1014      +/-   ##
==========================================
+ Coverage   72.39%   72.53%   +0.14%     
==========================================
  Files         364      364              
  Lines       46433    46750     +317     
==========================================
+ Hits        33616    33911     +295     
- Misses      12817    12839      +22

Impacted Files	Coverage Δ
include/cantera/base/AnyMap.h	`100.00% <ø> (ø)`
src/base/AnyMap.cpp	`89.59% <100.00%> (+0.54%)`	⬆️
src/kinetics/RxnRates.cpp	`89.83% <0.00%> (-3.58%)`	⬇️
src/kinetics/Reaction.cpp	`89.18% <0.00%> (-1.50%)`	⬇️
include/cantera/kinetics/RxnRates.h	`91.97% <0.00%> (-0.17%)`	⬇️
include/cantera/kinetics/Reaction.h	`100.00% <0.00%> (ø)`
include/cantera/kinetics/RateCoeffMgr.h	`100.00% <0.00%> (ø)`
src/kinetics/Kinetics.cpp	`83.62% <0.00%> (+0.04%)`	⬆️
src/kinetics/BulkKinetics.cpp	`88.63% <0.00%> (+0.13%)`	⬆️
src/kinetics/GasKinetics.cpp	`97.43% <0.00%> (+0.15%)`	⬆️
... 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 81cffde...32bf083. Read the comment docs.

speth · 2021-04-19T00:29:45Z

Well, I had a good portion of this implemented, as it is the logical extension to #984, but I guess you beat me to opening a PR. I pushed my work-in-progress on this to my set-user-data branch in case it's of any use.

ischoegl · 2021-04-19T01:27:28Z

@speth ... sorry, didn’t realize. While there is some redundancy, my PR doesn’t include user data. The reason I implemented it was that not being able to move data from Python to AnyMap forced me to use awkward workarounds, and I wanted to have this available for #995.

ischoegl · 2021-04-19T21:54:30Z

@speth ... I believe some of your ideas made the code simpler (especially using set operations). Shaved some 20 lines off, and the logic is clearer.

Simplify code using: * explicit type conversions * leverage of set properties Co-authored-by: Ray Speth <[email protected]>

Remove tests for unhandled cases that are now handled

speth · 2021-04-21T17:00:30Z

I updated the implementation on my branch, following on from what you have here. This version achieves a couple of things:

It works for any combination of nested containers (where the base types are convertible), including "ragged" nested arrays and higher-dimensional arrays.
The use of helper functions for populating the containers of like items avoids creating a bunch of unused containers on every call to python_to_anyvalue.

I hope you're willing to take this as a friendly amendment. If so, you can push those commits to this branch and we can continue the review from there (and of course you or @bryanwweber should provide a review of my changes).

ischoegl · 2021-04-21T18:56:38Z

@speth ... I did a hard reset to your branch, so I believe at this point both of our ideas are blended.

Overall, I have no objections to adopting the composite implementation, despite being somewhat sad to see isEmpty go, as I believe it to be more intuitive than isType[void], but that's not worth haggling over. Regarding 3-dim (and higher) arrays I had blocked those as I didn't see a need, and treating them as vector<AnyValue>> is different from the logical extension of vector<vector<vector<...>>>; I guess it's a theoretical question at the moment and would only have repercussions in the C++ layer anyways. Of course, they are legitimate values (just like ragged), but my NotImplementedError would have tabled these question. Overall, I'm generally more concerned about what features and interfaces are available than how things are implemented. To qualify the last statement: performance and code readability do matter, but that's not a concern here. Edit: those minor points are all resolved.

@bryanwweber ... it's up to you to review I guess, as @speth's friendly amendments are an improvement over my initial code, and I'm ok with them (Edit: my comments below are intended to clarify my points above). As an aside: @speth in case any changes come up, feel free to push directly to my branch.

interfaces/cython/cantera/utils.pyx

interfaces/cython/cantera/test/test_utils.py

bryanwweber · 2021-04-21T20:54:00Z

I'll take a look later this week after I finish this round of grading.

bryanwweber

Relatively minor changes here, thanks @ischoegl!

interfaces/cython/cantera/test/test_utils.py

bryanwweber · 2021-04-24T18:27:39Z

interfaces/cython/cantera/utils.pyx

+        itype = set()
+        for i in item:
+            if isinstance(i, dict):
+                itype.add(dict)
+            elif isinstance(i, list) or isinstance(i , tuple):
+                itype.add(list)
+            elif type(i) == bool:
+                itype.add(bool)  # otherwise bools will get counted as integers
+            elif isinstance(i, numbers.Integral):
+                itype.add(numbers.Integral)
+            elif isinstance(i, numbers.Real):
+                itype.add(numbers.Real)
+            else:
+                itype.add(type(i))
+
+        if itype == {numbers.Real, numbers.Integral}:
+            itype = {numbers.Real}


2 questions here:

Are the first two conditions needed? It seems as though itype.add(type(i)) would do well enough for those.

For the numbers, could you combine that condition? Since you reduce the set to numbers.Real at the end anyways. Or is there a use to having just numbers.Integral as the type?

It might be worth adding comments that answer these questions, for any future readers.

I can see from code below that the answer to question 2 is, yes there is a use to having numbers.Integral as the type.

Ad (1) ... I don't believe that the two conditions can be merged. Of course the second one can be simplified to isinstance(i, (list, tuple)).

@ischoegl is right that list + tuple check can be combined.

The reason for not just using itype.add(type(i)) is that I want to treat any type that inherits from dict as a dict, and likewise any types inheriting from list or tuple are essentially list-like for the purpose of serialization. I added a few comments to this effect.

interfaces/cython/cantera/utils.pyx

interfaces/cython/cantera/test/test_utils.py

ischoegl · 2021-04-24T20:08:00Z

@bryanwweber ... thanks for the review! I believe I resolved everything, so this should be ready to merge. The two comments where I left responses 'unresolved' pertain to code @speth (re-)wrote, so if there are remaining questions, I'll defer to him.

interfaces/cython/cantera/test/test_utils.py

* Clarify empty AnyValue * Fix docstrings * Address review comments

ischoegl · 2021-04-24T22:44:42Z

@bryanwweber ... I hope I was able to clarify the discrepancy between long int and long.

ischoegl · 2021-04-27T18:46:38Z

@bryanwweber ... any additional questions? I am mostly done rebasing #995 to include serialization (i.e. assuming that this would get merged before).

bryanwweber · 2021-04-27T19:16:57Z

@ischoegl Sorry, I was waiting to see if @speth wanted to respond to the other unresolved questions.

speth · 2021-04-27T22:15:20Z

I responded to two of the open threads. I didn't have anything to add to the third -- I think it's all set now, right?

ischoegl · 2021-04-27T23:06:35Z

@speth … thank you for your comments! All set from my side too.

ischoegl · 2021-04-30T16:51:50Z

🎉 ... thanks, @bryanwweber (and @speth)

ischoegl added 2 commits April 17, 2021 08:13

[Python] Convert simple Python types to AnyValue/AnyMap

b8c6e6f

[Python] Convert complex Python types to C++ AnyValue

e8d0ef0

ischoegl added Input Input parsing and conversion (for example, ck2yaml) Python labels Apr 17, 2021

[CI] Add unit tests for Python to AnyValue/AnyMap conversion

1afcdbe

ischoegl force-pushed the implement-dict-to-anymap branch from 787133f to 1afcdbe Compare April 17, 2021 15:38

ischoegl requested a review from a team April 17, 2021 15:55

ischoegl mentioned this pull request Apr 17, 2021

Serialization of root-level information in YAML #1013

Closed

ischoegl added 2 commits April 17, 2021 14:13

[Python] Improve Python API for empty AnyValue/AnyMap objects

d8bbf27

[Python] Improve error messages for conversions from Python to AnyValue

7787988

Also, add missing case for conversion of list of dictionaries

ischoegl mentioned this pull request Apr 18, 2021

Refactor reaction rate evaluators #995

Merged

8 tasks

ischoegl and others added 4 commits April 21, 2021 12:39

[Python] streamline implementation

111a91f

Simplify code using: * explicit type conversions * leverage of set properties Co-authored-by: Ray Speth <[email protected]>

Print nicer names for more AnyValue types

699091d

Use alternative implementation of python_to_anyvalue

a22b9a7

[Python] Test that AnyValue held types are correct

444d511

Remove tests for unhandled cases that are now handled

ischoegl force-pushed the implement-dict-to-anymap branch from 5774c40 to 9e12518 Compare April 21, 2021 18:37

ischoegl commented Apr 21, 2021

View reviewed changes

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

interfaces/cython/cantera/test/test_utils.py Outdated Show resolved Hide resolved

ischoegl force-pushed the implement-dict-to-anymap branch from 9e12518 to 453ee47 Compare April 23, 2021 01:21

bryanwweber requested changes Apr 24, 2021

View reviewed changes

ischoegl force-pushed the implement-dict-to-anymap branch from 453ee47 to 69b635b Compare April 24, 2021 19:55

ischoegl requested a review from bryanwweber April 24, 2021 20:08

bryanwweber reviewed Apr 24, 2021

View reviewed changes

interfaces/cython/cantera/test/test_utils.py Show resolved Hide resolved

bryanwweber reviewed Apr 24, 2021

View reviewed changes

interfaces/cython/cantera/test/test_utils.py Show resolved Hide resolved

[Python] Add clarifications and streamline code

87ad140

* Clarify empty AnyValue * Fix docstrings * Address review comments

ischoegl force-pushed the implement-dict-to-anymap branch from 69b635b to 87ad140 Compare April 24, 2021 22:43

ischoegl requested a review from bryanwweber April 24, 2021 22:45

[Python] Explain some logic in get_types

f6e8916

[Cython] document usage of 'long' instead of 'long int'

32bf083

bryanwweber approved these changes Apr 30, 2021

View reviewed changes

bryanwweber merged commit 3466422 into Cantera:main Apr 30, 2021

speth mentioned this pull request May 9, 2021

Enable modification of user input data fields in Python #1031

Merged

5 tasks

ischoegl mentioned this pull request Jun 9, 2021

Paper cuts in YAML serialization API #1053

Closed

11 tasks

ischoegl deleted the implement-dict-to-anymap branch June 22, 2021 21:28

speth mentioned this pull request May 3, 2022

Optional argument for Parameter Uncertainties in Cantera Input Files and Objects Cantera/enhancements#62

Closed

Uh oh!

Conversation

ischoegl commented Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

speth commented Apr 19, 2021

Uh oh!

ischoegl commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl commented Apr 19, 2021

Uh oh!

speth commented Apr 21, 2021

Uh oh!

ischoegl commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryanwweber commented Apr 21, 2021

Uh oh!

bryanwweber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bryanwweber Apr 24, 2021

Choose a reason for hiding this comment

Uh oh!

bryanwweber Apr 24, 2021

Choose a reason for hiding this comment

Uh oh!

ischoegl Apr 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

speth Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ischoegl commented Apr 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ischoegl commented Apr 24, 2021

Uh oh!

ischoegl commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanwweber commented Apr 27, 2021

Uh oh!

speth commented Apr 27, 2021

Uh oh!

ischoegl commented Apr 27, 2021

Uh oh!

ischoegl commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ischoegl commented Apr 17, 2021 •

edited

Loading

codecov bot commented Apr 17, 2021 •

edited

Loading

ischoegl commented Apr 19, 2021 •

edited

Loading

ischoegl commented Apr 21, 2021 •

edited

Loading

ischoegl Apr 24, 2021 •

edited

Loading

speth Apr 27, 2021 •

edited

Loading

ischoegl commented Apr 24, 2021 •

edited

Loading

ischoegl commented Apr 27, 2021 •

edited

Loading

ischoegl commented Apr 30, 2021 •

edited

Loading