Skip to content
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

Prelude to YAML serialization #795

Merged
merged 19 commits into from
Jan 21, 2020
Merged

Conversation

speth
Copy link
Member

@speth speth commented Jan 14, 2020

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

To make the upcoming PR which implements serialization to the YAML format a bit smaller and more manageable, I have extracted a number of commits which resolve some issues that I encountered and interface additions that I found were needed while implementing serialization, which can be considered separately.

Changes proposed in this pull request

  • Some additions to the AnyMap class, including:
    • equality comparisons
    • improved handling for entries which can be either single maps or lists of maps
    • a clear() method
    • automatic skipping of "hidden" fields (e.g. __file__) when iterating
    • generalization of handling for metadata applicable to an AnyMap tree
  • Fix a bug in the pure fluid models where calculating c_p and other similar properties slightly changes the state
  • Make instantiating phase types which reference other phase definitions possible when using a YAML input string (rather than an input file)
  • Implement Option 4 in Revise YAML species definitions to support multiple thermo models enhancements#20
    • Reorganize Debye-Huckel input data
    • Allow the equation-of-state field to allow multiple model options
  • Provide a way of determining the "canonical" name used by factories for instantiating an object, and make the names used in the YAML format canonical
  • Update pure fluid names to match YAML naming style
  • Fix adding YAML-defined reactions to CTI/XML-defined surface phases

@codecov
Copy link

codecov bot commented Jan 15, 2020

Codecov Report

Merging #795 into master will increase coverage by 0.1%.
The diff coverage is 86.31%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #795     +/-   ##
=========================================
+ Coverage   71.44%   71.54%   +0.1%     
=========================================
  Files         372      372             
  Lines       43952    44348    +396     
=========================================
+ Hits        31400    31730    +330     
- Misses      12552    12618     +66
Impacted Files Coverage Δ
src/thermo/VPStandardStateTP.cpp 81.63% <ø> (-0.25%) ⬇️
src/tpx/CarbonDioxide.h 100% <100%> (ø) ⬆️
src/thermo/HMWSoln.cpp 71.57% <100%> (-0.02%) ⬇️
src/kinetics/KineticsFactory.cpp 95% <100%> (ø) ⬆️
src/tpx/Heptane.h 100% <100%> (ø) ⬆️
test/thermo/thermoFromYaml.cpp 100% <100%> (ø) ⬆️
src/tpx/Sub.cpp 86.57% <100%> (+0.08%) ⬆️
src/thermo/Species.cpp 100% <100%> (ø) ⬆️
src/thermo/IdealSolidSolnPhase.cpp 60.16% <100%> (+0.08%) ⬆️
include/cantera/base/AnyMap.h 100% <100%> (ø) ⬆️
... and 17 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 f79d619...4447096. Read the comment docs.

For entries that are lists of maps in the general case but often only contain
one item, it is convenient to allow them to be provided as just a map. This
modifies methods used to work with vectors of AnyMap objects to handle the
case where the child object is just a map.
This allows for error checking at the time of adding the alias, as well as
providing a public interface for adding aliases.
The first phase in a Kinetics object is not necessarily the reacting phase
for Kinetics objects loaded from CTI/XML files, but is for YAML files.
@bryanwweber bryanwweber self-requested a review January 15, 2020 19:37
bryanwweber
bryanwweber previously approved these changes Jan 15, 2020
@bryanwweber bryanwweber dismissed their stale review January 15, 2020 19:38

Didn't mean to request/submit review

Ensure that the name returned by Sub::name() is a name that can be used to
reconstruct a substance of the same type.

Make phase names in liquidvapor.yaml match the substance model names.
After calculating properties using finite differences (e.g. cp), the fluid was
returned to its original state by setting the state in terms of pressure and
temperature. However, because setting the state using these variables is an
iterative calculation with some error tolerance, the state was not restored
exactly. By restoring the state based on the initial temperature and density,
the restored state is identical to the initial state.
Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, other than a few copy-editing things.

One of ``solvent``, ``charged-species``, ``weak-acid-associated``,
``strong-acid-associated``, ``polar-neutral``, or ``nonpolar-neutral``.
The types ``solvent``, ``charged-species``, and ``nonpolar-neutral`` can be
inferred automatically.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add something here about how these types are automatically inferred?

inferred automatically.

``weak-acid-charge``
Charge to use for species can break apart into charged species.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Charge to use for species can break apart into charged species.
Charge to use for species that can break apart into charged species.

the ``ideal-gas`` model is assumed.
A mapping or list of mappings. Each mapping contains an equation of state
model specification for the species, any parameters for that model, and any
parameters for interactions with other species. :ref:`sec-yaml-species-eos`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the reference intended to be a complete sentence here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants