PMEAL / OpenPNM

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

modes in set BCs should be better named #1997

Closed jgostick closed 2 years ago

jgostick commented 3 years ago

I'm thinking the following:

'force' 'hard/soft'

jgostick commented 2 years ago

Let's just keep them as they are.

jgostick commented 2 years ago

We should have the following:

Note that the above is NOT how things currently work.

jgostick commented 2 years ago

Here are the following definitions:

Three 'building blocks':

  1. add: Add new ones only to locations that don't already have BCs
  2. remove: Removes values from the given locations and leaves the rest
  3. clear: Remoes all the values from all locations

Two 'composite' options

  1. overwrite: Delete ALL old ones and replace new (clear followed by add)
  2. merge: Add new ones to all given locations regardless of what is already there, and keep all others (remove followed by add)

Merge will be the default.

jgostick commented 2 years ago

Some thought is requried regarding other BCs. I have implemented the proposed logic in the current _set_BCs method, which is a subclassed by the transport classes so works ok, but for something like advection diffusion the set_outflow bc does not inherit this. This means we need to implement the logic for each new/unique bc type. I don't know if there is a generic solution, but worth some thought.

jgostick commented 2 years ago

RE my above comment, I think we should implement a policy that all boundary condition arrays are named 'pore.bc_<type>'. This way the _set_BC method can find all types that are defined on the object by searching for the occurance of '.bc_' in the dict name, the ensuring that these are handled accordingly.

Note that I have already done this by creating a _get_bc_types method on the BC mixin, which returns a list of ['rate', 'value'] for instance. This list is iterated on when checking for BCs of other types, for instance.

jgostick commented 2 years ago

So, I turns out I don't like the name 'merge'. When describing how merge works I literally used the word overwrite, which makes me think it should be called overwrite.

Also, when I used 'overwrite' I was surprized that it didn't overwite the values in the 'other' bcs. So if I have some value BCs already set, then I want to set some rate BCs in the same pores, I would expect it to overwrite the value BCs. This is NOT what overwrite does at the moment. So...

Intially I thought about adding a 'force' mode, but after additional consideration, maybe a force=True flag would be better, to indicate that the given mode should be applied everwhere. So when force=False the following works

mode description
add Adds the given values to specified locations, but not in any locations which have a bc value of the given type. If force=True, then this will replace any values of the other types if present.
remove Removes the values of the given type from the specified locations, but only the specified type. If force=True, then this will remove any values of the other types if present.
clear Removes all BCs of all the given type from all locations. If force=True, then this will also remove any values of the other types if present.
overwrite Adds the given values to the given locations whether there are values of the given type defined there already. If force=True, then this will also overwrite any values of the other types if present.
jgostick commented 2 years ago

I have removed the force argument on that basis that it added a lot of clutter to the method and required careful reading of the docstring. Basically it was too complicated considering who little it would be used. Basically, the user just needs to set the BCs correctly to start with, and the powers of the force are not needed.

jgostick commented 2 years ago

After too much overthinking we have: