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

flexible data structure for model-specific parameters #443

Merged
merged 3 commits into from
Mar 26, 2017

Conversation

speth
Copy link
Member

@speth speth commented Mar 3, 2017

This PR adds a new type-flexible data structure (AnyMap) for storing model-specific data. It can be used instead of XML_Node trees to store model parameters without needing to create a family of classes with various specializations just to hold this data.

Here, an instance of this data structure is attached to the Species class to hold species properties relevant to the Debye-Huckel model (and some of the other electrolyte models) so that DebyeHuckel objects can be instantiated without requiring XML phase and species definitions.

I think it would also make sense to use this same location to store the standard state model parameters for each species with the Species object so that PDSS objects could be automatically created rather than needing to be manually instantiated and added to phase objects that require them (see some of the tests in phaseConstructors.cpp for an example of the current approach).

@speth speth requested a review from bryanwweber March 3, 2017 05:42
@speth speth changed the title Dh xml flexible data structure for model-specific parameters Mar 3, 2017
@codecov
Copy link

codecov bot commented Mar 3, 2017

Codecov Report

Merging #443 into master will increase coverage by 0.2%.
The diff coverage is 81.22%.

@@            Coverage Diff            @@
##           master     #443     +/-   ##
=========================================
+ Coverage   58.91%   59.12%   +0.2%     
=========================================
  Files         381      384      +3     
  Lines       40229    40340    +111     
  Branches     6681     6679      -2     
=========================================
+ Hits        23701    23850    +149     
+ Misses      14564    14528     -36     
+ Partials     1964     1962      -2
Impacted Files Coverage Δ
src/thermo/Elements.cpp 95.7% <100%> (+0.02%)
include/cantera/thermo/DebyeHuckel.h 100% <100%> (+100%)
test/general/test_containers.cpp 100% <100%> (ø)
include/cantera/base/AnyMap.h 100% <100%> (ø)
src/thermo/Species.cpp 75% <50%> (-4.17%)
src/thermo/DebyeHuckel.cpp 41.84% <61.25%> (+3.36%)
src/base/AnyMap.cpp 82.22% <82.22%> (ø)
test/thermo/phaseConstructors.cpp 99.15% <95.34%> (-0.85%)
... and 3 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 2c3512c...abba568. Read the comment docs.

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.

@speth This looks good to me, and very useful. I like that the structure/syntax of the AnyMap mirrors the expected format from the YAML input file, and also that the syntax is very close to Python dictionaries.

A few questions, just to make sure I understand:

Is the plan that XML/CTI/YAML files will be used to generate an AnyMap, which is then used to instantiate various objects? That seems like it would fantastic!

Just to make sure I understand, std::map only allows one type of key and one type of value, whereas the AnyMap allows any combination if either?

@speth
Copy link
Member Author

speth commented Mar 26, 2017

Right, the idea is that a YAML file will be used to generate an AnyMap. CTI files will still have to be converted to YAML first, since there's no other way to parse the CTI file. You could also support other input file formats by just writing the code to parse them and create the appropriate AnyMap.

For XML, there are a couple of ways of handling it, depending on how long we want to support the existing XML format. To maintain longer-term support, XML files could also be parsed into AnyMap objects (instead of XML_Node trees), or if we decide the next version will be the last to support XML directly, I'd be more inclined to just leave the existing XML input file handling mostly as-is.

The AnyMap class requires keys to be strings, but allows values to be of different types.

@speth speth merged commit bfdc2b9 into Cantera:master Mar 26, 2017
@speth speth deleted the dh-xml branch March 26, 2017 03:42
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