PMEAL / OpenPNM

A Python package for performing pore network modeling of porous media
http://openpnm.org
MIT License
442 stars 175 forks source link

_get_domain_area in GenericTransport doesn't work for 2D networks #1017

Closed TomTranter closed 6 years ago

TomTranter commented 6 years ago

For nets that are 1 pore thick the following line breaks as the points are essentially 1D and need to be at least 2D hull_in = ConvexHull(points=inlets[:, Nin])

The bug was found when calculating the effective prop for a 2D network that needs the area and length of the domain.

The line is part of a check about the inlet and outlet faces having the same area which could be changed or skipped for 2d nets.

Alternatively, the user could be asked for their own value for A and L in the calc as they are a little subjective anyway... do you take the pore coordinates or the bounding box?

Thirdly, something which might be quite useful is to have a function that takes a square or rectangular section of any network and does the calculation on this sub-net. Would be handy for non-cubic and non-square domains and also aid study's of REVs. Would maybe involve too many options though and obviously more work than the other two alternatives

@jgostick what do you think?

jgostick commented 6 years ago

The transport algs have an domain_area and domain_length attribute that the user can set (eg. alg.domain_area = 22). If they are not set then they return None, which cues the automatic attempt to estimate them. I also think that calc_eff_diffusivity should optionally accept domain_area and domain_length when the user calls it, to make it extra clear that these values should be specified.

As for 2D networks, I don't really know what the meaning of area is...?

TomTranter commented 6 years ago

Are these attributes used anywhere other than in calc_eff_prop() ? They don't seem to be. Doesn't it make more sense just to pass in these values to that function. Also, looking at the code for the _get_domain... funcs, these are quite a strict set of rules that will only really work on square cubic network using all the pores on the opposing faces for inlets and outlets. The Fickian alg doesn't have to be setup that way at all so this function will probably not run in a large number of cases. Might it be worth removing these funcs from these algs and having another algorithm that sets up Fickian the right way. It could have extra logic that could handle random and non-square networks by possibly making a copy and trimming pores outside a bounding box then stitching a new set of 'perfect' boundary pores. Will the effective prop even work if the user has specified rate B.C instead of value

jgostick commented 6 years ago

Yes, it would make sense to remove the attribute and have them passed as arguments. These could be optional and estimated if not given using the present approach.

The strictness of the algorithm is necessary (I think).

TomTranter commented 6 years ago

I understand why the rules are there for the purposes of the calculation but I'm saying does it make sense to include it and take options and settings from a much more general algorithm when it will break so often... I take your point about the alg expecting the network to be set up correctly although maybe we should improve the doc string on the effective prop functions to explain in more detail how things need to be set up

TomTranter commented 6 years ago

In addition I think we should make the messages logger warnings instead of exceptions. The different face area check triggers for VoronoiFibers because it's using pore coords whose centers don't span the same area but actually the throats they connect to do form faces that cover the same or very similar area - the won't be exactly the same

xu-kai-xu commented 5 years ago

seems you gonna remove the domain attribute in the future version, and this attribute(domain_area, domain length) is not very broadly used.

jgostick commented 5 years ago

Interesting....I will open an issue and think of a way to fix this. We do want to support 2D networks fully, so this is still needed.