OpenPNM icon indicating copy to clipboard operation
OpenPNM copied to clipboard

Change models to only accept one keyword for each prop (temperature, instead of pore_temperature and throat_temperature)

Open ma-sadeghi opened this issue 4 years ago • 6 comments

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 ?

ma-sadeghi avatar Aug 12 '20 15:08 ma-sadeghi

There is some value to that suggestion for sure

jgostick avatar Aug 12 '20 15:08 jgostick

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.

jgostick avatar Mar 01 '21 15:03 jgostick

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.

jgostick avatar Sep 08 '21 15:09 jgostick

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.

jgostick avatar Dec 15 '21 20:12 jgostick

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?

jgostick avatar Dec 15 '21 20:12 jgostick

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.

ma-sadeghi avatar May 11 '22 16:05 ma-sadeghi

We have discussed this and decided to close it for the following reasons:

  1. 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.
  2. The ability to specific both pore and throat properties might be useful in some cases
  3. The dependency handler will need to be massively fixed up to handle viscosity='viscosity'
  4. 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
  5. The model author has the ability to accept just viscosity='viscosity' if they really want

jgostick avatar Aug 22 '22 13:08 jgostick