Make Sim1D/Domain1D and SolutionArray usage more consistent#1995
Make Sim1D/Domain1D and SolutionArray usage more consistent#1995ischoegl merged 24 commits intoCantera:mainfrom
Sim1D/Domain1D and SolutionArray usage more consistent#1995Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1995 +/- ##
==========================================
- Coverage 75.42% 75.24% -0.19%
==========================================
Files 454 454
Lines 56554 56756 +202
Branches 9334 9373 +39
==========================================
+ Hits 42654 42704 +50
- Misses 10746 10873 +127
- Partials 3154 3179 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ischoegl
force-pushed
the
rename-lambda
branch
from
fd23b9e to
fac152f
Compare
ischoegl
force-pushed
the
rename-lambda
branch
4 times, most recently
from
7a9f6bd to
b84a8fa
Compare
ischoegl
changed the title
Sim1D and SolutionArray usage more consistent
ischoegl
force-pushed
the
rename-lambda
branch
from
b84a8fa to
427c6d6
Compare
ischoegl
changed the title
Sim1D and SolutionArray usage more consistentSim1D/Domain1D and SolutionArray usage more consistent
ischoegl
force-pushed
the
rename-lambda
branch
4 times, most recently
from
56dd64f to
1f547a2
Compare
ischoegl
force-pushed
the
rename-lambda
branch
from
7836bbf to
fb34cb7
Compare
|
@speth ... This should be ready for a review. What started with a simple bug fix ended up being a broader maintenance job. The work has the side effect of streamlining access for CLib, which should benefit future updates of the MATLAB oneD portion. |
speth
left a comment
speth
left a comment
There was a problem hiding this comment.
Thanks for the work on this, @ischoegl. It certainly expanded in scope well beyond resolving the issue with accessing the lambda component. Most of the changes here make sense to me, with the exception of "rolling back convenience access via Sim1D".
- I'm on the fence about the use of
vector<double>&as the exchange medium ofDomain1D::get/setValuesand related methods. While it avoids the unsafe raw pointer usage, it is somewhat inflexible, as seen by the need to create temporaryvectors where these methods are used both in the Python interface andCLib. I guess the longer-term solution here is to usestd::span, as described in Cantera/enhancements#237. - Shifting the Python
set_profileand related methods out ofSim1Dto the underlyingDomain1Dseems reasonable, and ultimately leads to some good simplifications of the code. However, I'm not keen on deprecating the methods for accessing the grid, temperature, velocity, etc. arrays for theflamedomain via the parentSim1Dobject. I think this just makes some of the most common data to be accessed more cumbersome to work with. Also, it seems strange to leave some data accessible via theSim1Dobject (such as mass fractions) while moving others to the flow domain. I do think the new, more descriptive names for the solution components are a beneficial change.
| auto fullName = findInputFile(fname); | ||
| m_file = make_unique<h5::File>(fullName, h5::File::ReadOnly); |
There was a problem hiding this comment.
This seems unrelated to the rest of the PR. When would you want to read simulation results from a directory on the data path?
There was a problem hiding this comment.
This will be useful if you keep your YAML input file and simulation data in a folder separate from the code used to analyze your data. In this case, you'd set add_data_directory and have things accessible; if I split hair, this avoids having serialized YAML accessible while HDF5 isn't?
There was a problem hiding this comment.
I'll grant the point on consistency. Though if you're saving simulation data to a folder, you need to specify the particular output path -- it's not as though Cantera can/should direct output to an arbitrary directory on the CANTERA_DATA path.
ischoegl
force-pushed
the
rename-lambda
branch
3 times, most recently
from
e05921d to
026cb53
Compare
This removes inconsistencies in naming convention, where the snake_case 'spread_rate' is an outlier. Note that the other component specific to axisymmetric flows is also renamed in the same PR.
ischoegl
force-pushed
the
rename-lambda
branch
2 times, most recently
from
bf3d735 to
3c6bb0d
Compare
|
@speth ... I believe I addressed all concerns. |
2 participants
Changes proposed in this pull request
Sim1D/Domain1Din Python more consistent withSolutionArray.lambdafrom Python. The component is internally renamed toLambda, but it can still be accessed as before through an 'alias' mapping.getValues/setValuesforDomain1D; new methods are also made available to CLib.L->radial_pressure_gradient,E->electric_field,Uo->oxidizer_velocityand move them toDomain1D.Sim1D.Sim1Dcontent viaSolutionArrayNote: While properties are made available for
Domain1D, convenience getters forSim1Dthat are not renamed remain unchanged. Future changes toSolutionArrayandDomain1Dshould provide access to various calculated results via CLib; these changes go well beyond the scope of this PR.If applicable, fill in the issue number this pull request is fixing
Closes #1865
If applicable, provide an example illustrating new features this pull request is introducing
Fixes
SolutionArray:Makes
Domain1Dmore consistent withSolutionArray:Checklist
scons build&scons test) and unit tests address code coverage