-
-
Notifications
You must be signed in to change notification settings - Fork 349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Solutionarray summary #1462
Solutionarray summary #1462
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1462 +/- ##
==========================================
+ Coverage 69.90% 69.98% +0.07%
==========================================
Files 377 377
Lines 57349 57586 +237
Branches 19201 19326 +125
==========================================
+ Hits 40092 40301 +209
- Misses 14706 14726 +20
- Partials 2551 2559 +8
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
88b7938
to
1e2a0b9
Compare
317689c
to
387a77c
Compare
@speth and @bryanwweber … while there are some loose ends for related issues I just documented in Cantera/enhancements#166, the formatting itself is ready for a review. PS: the force push mainly took care of an edge case. |
387a77c
to
0424814
Compare
dc80b11
to
0424814
Compare
Loose ends uncovered here are resolved by #1464. PRs are almost completely independent, and I will resolve merge conflicts after merging whatever gets reviewed first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ischoegl, this is a nice addition, and the implementation looks good to me.
I had one suggestion that I think might significantly extend its usefulness.
Looking at the way the Python method works on sliced solution arrays, would it make more sense to have a "include" list rather than a set of components to skip? With control over the column ordering, this would almost immediately provide a more attractive and flexible alternative to the output of Domain1D.show()
. For example, with a FreeFlame
object f
:
print(f.to_solution_array()('H2', 'O2', 'H2O')[::5].info(rows=30, width=100))
Gives the output:
T D H2 O2 H2O grid velocity
0 300.000 1.338612 9.47832e-03 0.1367637 7.40673e-19 0.0000000 0.727045
10 300.006 1.339040 9.45641e-03 0.1367666 3.52939e-08 0.0200625 0.726814
20 314.689 1.303354 8.10736e-03 0.1367547 3.88906e-04 0.0207891 0.746713
30 362.366 1.149201 7.04860e-03 0.1358127 2.22271e-03 0.0208711 0.846876
40 415.421 1.010257 6.43539e-03 0.1343538 4.52006e-03 0.0209062 0.963350
50 535.915 0.790700 5.52333e-03 0.1304149 1.00483e-02 0.0209531 1.230847
60 741.255 0.576925 4.34133e-03 0.1222623 2.04189e-02 0.0210117 1.686930
70 951.846 0.451853 3.20124e-03 0.1112773 3.29602e-02 0.0210703 2.153872
80 1167.908 0.369349 1.99580e-03 0.0957110 4.85175e-02 0.0211406 2.634995
90 1407.764 0.306713 7.83098e-04 0.0754498 6.65237e-02 0.0212578 3.173101
100 1588.912 0.272250 3.08697e-04 0.0658318 7.50175e-02 0.0214453 3.574766
110 1719.334 0.252370 1.70897e-04 0.0635787 7.82862e-02 0.0217969 3.856372
120 1810.158 0.240400 6.51822e-05 0.0630420 8.06377e-02 0.0226875 4.048379
130 1850.402 0.235543 1.26681e-05 0.0632889 8.18752e-02 0.0255000 4.131865
[14 rows x 7 components; state='TDY']
Comes pretty close already, with the exception that you would want the grid
column to come first (and probably velocity
before species).
Thanks for the review, @speth!
Improving this exact part of the API was actually a major motivation for this PR, but #1464 was in the way. The include list makes sense, and I will adopt it.
The only reason this doesn't have the correct ordering already is that the 'vestigial' Python 1D API is in the way of the C++ implementation. From that viewpoint, I'd suggest to finalize/merge #1464 first, so I can rebase this one and can test accordingly. |
0424814
to
722a14e
Compare
@speth ... rebased and adopted the two minor suggestions. With #1464 merged, we now have
I.e. ordering is indeed corrected, and At this point
and
produce identical output. |
69e1a12
to
2a02bb3
Compare
@speth ... I updated the C++ core code to use an include list rather the exclude list, and added the following capability to the Python API:
which I believe is what you suggested. PS: While I am keenly aware that this is useful for the PPS: last force-push was to remove a couple of imports that weren't needed any longer. |
2a02bb3
to
4c425bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ischoegl. Just a couple of small comments from my end.
I wasn't intending to suggest that using this to implement display of 1D domains should be part of this PR, just that the positive include list would make this easier eventually.
b7bae9d
to
36051a0
Compare
@speth ... thanks for the comments. Both suggestions are addressed. (Update: apart from needing to update unit tests 😥) |
36051a0
to
adc38f5
Compare
Changes proposed in this pull request
This PR adds a utility function
SolutionArray::info
, which provides a concise summary of the content. The styling is inspired by pandas'DataFrame.info
, although it is implemented in C++ and thus available for all API's. Under the hood, the C++ libraryfmt
is used to format content.In addition, this PR fixes a glitch in the order ofSolutionArray::componentNames
, where the native storage modeTDY
was alphabetically sorted asDTY
(same fix is also added in #1464).If applicable, provide an example illustrating new features this pull request is introducing
and
Checklist
scons build
&scons test
) and unit tests address code coverage