mutationpp / Mutationpp

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

`use_transport` flag in mixture inputs appears to do nothing #258

Closed mgoodson-cvd closed 3 weeks ago

mgoodson-cvd commented 4 months ago

The documentation indicates that there is a flag in the mixture file called use_transport that controls whether or not the transport properties are loaded. However, this flag is never checked or used to set anything.

There is also a flag in the MixtureOptions class called m_load_transport, which I'm assuming was supposed to be what gets set by this input. However, this flag also does nothing and does not control whether or not the transport is loaded.

We should either a) remove this input from the documentation, or b) make it work as intended. Option a is certainly easiest, but there are instances when b would be useful.

jbscoggi commented 4 months ago

Hey @mgoodson-cvd, it looks like I did want to add this feature a long time ago and probably ran into a bad design choice for the Mixture class and gave up. The problem is that Mixture inherits from Transport instead of owning a Transport object. The main reason for doing this was avoid having to provide a ton of wrapper methods in Mixture that just called the transport object or to have to redirect through a secondary "transport()" function to call transport properties. For example, mixture.transport().viscosity(). In hindsight, using composition would definitely have been a better design choice and I would use composition over inheritance today. One nice benefit would be that you could choose not to load a transport database until you needed it, or better yet, use dependency injection to give a mixture a transport database that could be anything matching the interface. The workaround for the current design would be to pass the job of loading the transport database to the Transport class as an optional boolean parameter (coming from MixtureOptions) and it would need to manage the loading of the CollisionDB object. With all that said, I think it makes the most sense to go with your option a) and remove the flag and its reference in the documentation. If it's decided that it is indeed worthwhile in the future, we can make a new feature request and implement it then.

mgoodson-cvd commented 4 months ago

@jbscoggi Got it, thanks for the info. Agree on the inheritance vs composition, but that feels beyond my scope at the moment. The number of use cases that would need thermo but not transport seems small to me, so I will take your advice and remove the flag.