OpenPNM
OpenPNM copied to clipboard
Change models to only accept one keyword for each prop (temperature, instead of pore_temperature and throat_temperature)
Many of our methods' arguments include both pore and throat properties, such as pore_diffusivity
and throat_diffusivity
. Is that really necessary? I'm proposing, for the benefit of brevity and cleaning up the argument list, to remove most of these references and only leave out those that would otherwise introduce significant errors if interpolated.
What do you think @jgostick ?
There is some value to that suggestion for sure
not sure about interpolating, but we should change args to remove the throat_
and pore_
business. Just diffusivity
and the function looks up 'pore.' + diffusivity'
and 'throat.' + diffusivity'
. This would be a break change since it alters the api of the models.
For any models that NEEDS different values, they are welcome to use pore_blah
and throat_blah
.
NOTE: If we do this, which we should, we need to update the dependency handler which at present looks for 'pore.blah', so it will need to see 'blah' and realize what was meant. The dependency map may also need to be updated.
Another problem with this is that we are now allowing 'pore_temperature=373.0' to return 373. So if we do T = phase['pore.' + temperature']
inside the code, this will break.
So, it looks like the dependency handler and the numeric return will both be broken with this change. I'm thinking perhaps we should keep things as they are?
I'm reopening this issue as it simplifies the code quite a bit. Take `` model for instance:
def hagen_poiseuille_power_law(target,
pore_area="pore.area",
throat_area="throat.cross_sectional_area",
pore_viscosity_min="pore.viscosity_min",
throat_viscosity_min="throat.viscosity_min",
pore_viscosity_max="pore.viscosity_max",
throat_viscosity_max="throat.viscosity_max",
conduit_lengths="throat.conduit_lengths",
size_factors="throat.hydraulic_size_factors",
pore_consistency="pore.consistency",
throat_consistency="throat.consistency",
pore_flow_index="pore.flow_index",
throat_flow_index="throat.flow_index",
pore_pressure="pore.pressure",
throat_pressure="throat.pressure"):
...
could become:
def hagen_poiseuille_power_law(target,
pore_area="pore.area",
pore_viscosity_min="pore.viscosity_min",
pore_viscosity_max="pore.viscosity_max",
conduit_lengths="throat.conduit_lengths",
size_factors="throat.hydraulic_size_factors",
pore_consistency="pore.consistency",
pore_flow_index="pore.flow_index",
pore_pressure="pore.pressure"):
...
or possibly even:
def hagen_poiseuille_power_law(target,
area="pore.area",
viscosity_min="pore.viscosity_min",
viscosity_max="pore.viscosity_max",
conduit_lengths="throat.conduit_lengths",
size_factors="throat.hydraulic_size_factors",
consistency="pore.consistency",
flow_index="pore.flow_index",
pressure="pore.pressure"):
...
or even:
def hagen_poiseuille_power_law(target,
area="area",
viscosity_min="viscosity_min",
viscosity_max="viscosity_max",
conduit_lengths="conduit_lengths",
size_factors="hydraulic_size_factors",
consistency="consistency",
flow_index="flow_index",
pressure="pressure"):
...
I like option 3 the best.
We have discussed this and decided to close it for the following reasons:
- The values all have defaults, so there is not that much overhead on the part of the user since they will only change 1 or 2 at most.
- The ability to specific both pore and throat properties might be useful in some cases
- The dependency handler will need to be massively fixed up to handle
viscosity='viscosity'
- The model regeneration machine will still need to handle the case of 'pore.viscosity', so we would need to support both ways of doing things
- The model author has the ability to accept just
viscosity='viscosity'
if they really want