search

PMEAL / OpenPNM

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

DelaunayVoronoiDual includes too many vertices in facets and hulls #858

Closed TomTranter closed 6 years ago

TomTranter commented 6 years ago

I've been testing the new VoronoiFibers material class which subclasses DelaunayVoronoiDual and have been debugging an issue all day. I tracked it to the call to create_adjaceny_matrix which should include a parameter of drop_zeros=True in version 2. I haven't checked whether this bug is also present in version 1. The line of code is the same but maybe the default argument of the create_adjacency_matrix has changed. It is probably messing up @Zohaib-Atiq 's simulations. For me the bug resulted in the throat facets having too many vertices which meant that they weren't planar and when I was doing calculations to get their properties such as normal vector it was wrong. I assume it will also be causing the throat areas to be calculated incorrectly in some other models. Good news is it's a simple fix. The same line is in both functions to find_throat_facets and find_pore_hulls and should be changed in both

TomTranter commented 6 years ago

Yeah, as I suspected the default changed at some point on GenericNetwork. V1.x should be ok. So if @Zohaib-Atiq is using V2 then it would be worth re-running the simulations

Zohaib-Atiq commented 6 years ago

@TomTranter I dropped DelaunayVoronoiDual a month ago. I was using V1.6 at that time to avoid V2 crashes. If V2 is giving the same result as V1.6 now after bug fix then I believe problem still persists. During this time I have prepared a couple of pore-scale conductance models which give good results on SNOW. I haven't tried it for DelaunayVoronoiDual yet. Maybe they can give better results I guess.

TomTranter commented 6 years ago

@jgostick this bug is fixed on my current Voronoi changes branch which is ready to merge. However, even if the bug had not been fixed it would pass the current tests because a logger error is thrown rather than an exception. We should probably have another think about the logger...