mutationpp / Mutationpp

The MUlticomponent Thermodynamic And Transport library for IONized gases in C++
GNU Lesser General Public License v3.0
103 stars 58 forks source link

Redesign of Millikan-White model classes and OmegaVT #173

Closed jbscoggi closed 3 years ago

jbscoggi commented 3 years ago

This is a redesign of the OmegaVT class and associated Millikan-White model classes in response to #171. In addition to now using the correct implementation for the Millikan-White model as discussed in #171, the redesign provides additional improvements, including:

  1. Separation of relaxation time model and energy model into separate classes (MillikanWhiteModel and HarmonicOscillator)
  2. Separation of model behavior and creation for both new model types using builder pattern to create model instances from XML databases
  3. Separation of default model data for the Millikan-White model from data non-default data provided in a database via the MillikanWhiteModelData class
  4. OmegaVT now manages generic "vibrator" objects which could implement different relaxation or energy models in the future and the energy transfer source term is now computed in a clear way following the generic Landau-Teller form, regardless of the relaxation time model used
  5. The redesign also allows an easy extension of the OmegaVT class for situations where multiple vibrational temperatures are used, for example by providing a list of vibrator species names to be managed by the specific source term.

In addition to the design improvements, there is a significant increase in documentation, including clearly citing the sources for default data and model equations in the literature. Finally, several test cases have also been added to test each new class as separately as possible. Note, there is still some ugly coupling with Thermodynamics that is basically imposed by needing state information to compute the relaxation rate. This could be improved in the future but is out of the scope of this redesign.

Finally, there is a small change to the name of the test case "Bug #139 fix: vibronic energies unchanged after setState()" because this test was not actually being picked up by the ctest suite as due to a problem in the test name parsing.

codecov[bot] commented 3 years ago

Codecov Report

Merging #173 (0ca0638) into master (8c1c129) will increase coverage by 0.09%. The diff coverage is 95.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #173      +/-   ##
==========================================
+ Coverage   71.13%   71.22%   +0.09%     
==========================================
  Files         133      135       +2     
  Lines        8961     8972      +11     
==========================================
+ Hits         6374     6390      +16     
+ Misses       2587     2582       -5     
Impacted Files Coverage Δ
src/transfer/MillikanWhite.cpp 90.24% <90.24%> (-0.17%) :arrow_down:
src/thermo/HarmonicOscillator.cpp 96.66% <96.66%> (ø)
src/thermo/HarmonicOscillator.h 100.00% <100.00%> (ø)
src/transfer/MillikanWhite.h 100.00% <100.00%> (ø)
src/transfer/OmegaCE.cpp 100.00% <100.00%> (ø)
src/transfer/OmegaCElec.cpp 100.00% <100.00%> (ø)
src/transfer/OmegaCV.cpp 85.18% <100.00%> (ø)
src/transfer/OmegaET.cpp 100.00% <100.00%> (ø)
src/transfer/OmegaI.cpp 100.00% <100.00%> (ø)
src/transfer/OmegaVT.cpp 100.00% <100.00%> (+2.04%) :arrow_up:
... and 7 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 8c1c129...0ca0638. Read the comment docs.

jbscoggi commented 3 years ago

I have made some slight modifications to

  1. further reduce compilation dependencies in MillikanWhite.h
  2. bump version 1.0.5 -> 1.0.6
  3. change C++ standard to C++14 in order to make use of std::make_unique.

Note that going from C++11 to C++14 should not create any issues because all main compilers should support 14.

jbscoggi commented 3 years ago

Hey @Khben, @grgbellasvki mentioned you would have a look at this PR since you have experience implementing the model in US3D. I went ahead and added you to the reviewers. If you are happy with it, I think we can merge. Thanks!

jbscoggi commented 3 years ago

I realized I forgot to take care of all the corner cases when loading data in this PR. When data doesn't exist, the database classes are simply returning the default (generally empty) objects. While this will allow the program to continue, it is probably not a very good idea to simply hide the fact that some data has not been found because the user is not aware that the model is not performing as they probably expect it to. On the other hand, there are instances in which you want the program to ignore missing data because sometimes it's not easy to find the data you need to fully specify the mixture. For example, when Millikan-White data is not provided, the default parameters are assumed based on the formulas provided by Millikan and White given the species' molecular weights and the vibrator's characteristic vibrational temperature. I think this is probably desired behavior for most cases. However, if the vibrational temperature is not found in the species.xml file, then this means there is not enough data to compute the default, which should probably result in an error. At a bare minimum, I think we should probably have some sort of logging system to notify users that these decisions are being made behind the scenes for them which can be turned on or off depending on their preference. For example, using a logging system to print these types of events when you run checkmix could be useful...