Thermodynamic API Consistency #240
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm a big fan of the goal of the changes, but I don't have time to go through line-by-line this week. Could you throw in a few unit tests before we merge? If not then we can settle for testing via GRINS for the short term. |
This will serve as the base class for thermodyanmics objects that are expected to provide both "micro" thermo quantities, e.g. cv_trans, cv_rot, etc. as all as "marco" thermo quantities, e.g. cp, cv. This base class will be abstract and merely provide common functionality and define the API for subclasses.
This is step 1 of refactoring StatMechThermodynamics, only moving ChemicalMixture reference to base class at this point.
Move common functions to MicroThermoBase from StatMechThermodynamics. These implementations are the same regardless of subclass for the ideal gas case, so just put them in the base class so we can remove the duplicate implementations in IdealGasMircoThermo in subsequent commits.
cv_vib() and cv_vib_over_R APIs have been moved to MicroThermoBase and there are now cv_vib_impl and cv_vib_over_R_impl in each of StatMechThermodynamics and IdealGasMicroThermo. I've made the *impl methods private so we need to friend the base class in that case. This gets us to the point where we have a uniform interface for everything that was in IdealGasMicroThermo. We now need to extend IdealGasMicroThermo (and MicroThermoBase) to get a uniform API for all macro thermo and micro thermo the user might want to use between the two MicroThermoBase subclasses.
Adds interface for cv_el and moves existing StatMechThermodynamics implementation to impl method. Implements IdealGasMicroThermo cv_el, which is just zero by assumption for IdealGasMicroThermo.
Since we're defining an API for both macro and micro thermo to be used by the user, this name makes more sense for the class, I think. This still may not be the right final design (e.g. we want want to separate macro and mirco thermo), but we'll go with this for now.
Just a common header to put useful functions for the testing suite. This just has a relative error function, currently.
Currently only testing test_cv_trans_over_R
Currently only testing test_cv_trans_over_R.
Both for StatMechThermo and IdealGasMicroThermo
For both StatMechThermo and IdealGasMicroThermo
For both StatMechThermo and IdeanGasMicroThermo classes. This is the last of the "common" tests. Other tests will now depend on the subclass.
This way, we can easily reuse these functions.
No new methods, just splitting up the testing class, redoing the macros to ready for doing cv_vib testing
We should really put all of these in one place and just subclass that class.
Both versions of the curve fit, NASA7 and NASA9.
This was a standalone test that has now been replaced by a CppUnit-based version that has more complete coverage.
This was probably leftover from the API redux during 0.3.0->0.4.0. It was not being built or used.
|
I've migrated a lot of testing here, but not quite done. Need coverage for And I'm glad I resisted the urge to say "we have already have coverage from old style tests, let's merge" and instead said "let's migrate these tests to CppUnit and make sure we have coverage" since it uncovered #241. Thanks for the prod @roystgnr. |
This is easy since, by assumption, it's just zero.
Note that this isn't standing up to testing in long double precision. My guess is the exp implementation, but that's just a guess. And seeing that we'll probably hit the same with cv_el, I just created a second block of testing classes that won't get hit with long double.
Refactored the testing slightly to reuse common functionality. We should now be covering all of cv_vib functions for all MacroMicroThermoBase subclasses.
Duh. Just use a base class pointer and new it in the subclass. Then, the functions that are common implementations can just sit there in the base testing class and only exist once instead of multiple times.
This completes coverage for the MacroMicroThermoBase functions.
This should now cover all the old testing we have in place.
It has been replaced by (more complete) CppUnit-based testing.
|
OK, all the methods in |
This was referenced May 12, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #238
This uses a CRTP pattern to define an API for "MacroMicro" thermo objects. That is, objects that provide both "macro" thermo functions (e.g. cp) and "micro" thermo functions (e.g. cv_trans). Doing this also removed some duplicate code from
IdealGasMicroThermo. It may not be appropriate to have these in a combined object like this, but we can revisit when we want to start thinking about multi-temperature models.I'd like to merge quickly because we'll consume this in GRINS right away.