(WIP) Combine boundary condition setting so that both q and aux are available to these functions

[mandli]

Please see issue #92 for the relevant discussion as to what this PR is supposed to solve and some of the issues with this implementation.

[ketch]

Looks great. Thanks for resurrecting this!

My thoughts:

• Wall BCs for the aux variables should not just extrapolate; I can't really think of a situation where that makes sense except when it is equivalent to reflected extrapolation.
• You still have a lot of old code commented out.
• We could just move to a framework where a single BC routine needs to be specified for both q and aux. This would be advantageous when one wants to extrapolate both, reflect both, make both periodic, or do something custom for both. It would be disadvantageous when one wants to use a built-in BC for one of them and a custom BC for the other. Overall, it seems like an improvement to me -- though it is even less backwards compatible. Perhaps this should go into a 6.0 branch?

[mandli]

• Wall BCs for the aux variables should not just extrapolate; I can't really think of a situation where that makes sense except when it is equivalent to reflected extrapolation.

Upon further reflection I think you are right about this, I will have to implement something that checks the type of array input (probably using *args).

• You still have a lot of old code commented out.

Yeah, sorry, was being lazy when comparing the old code to the new. I will be removed.

• We could just move to a framework where a single BC routine needs to be specified for both q and aux. This would be advantageous when one wants to extrapolate both, reflect both, make both periodic, or do something custom for both. It would be disadvantageous when one wants to use a built-in BC for one of them and a custom BC for the other. Overall, it seems like an improvement to me -- though it is even less backwards compatible. Perhaps this should go into a 6.0 branch?

I originally was thinking along these lines but then thought it would be better to do the option that leaves the most control and least duplication of code available. We could provide convenience routines to do this. One thing of note, since both qbc and auxbc are passed to the custom routines it is possible just to set both and not worry about having two functions. Also a lambda function could be used to even better effect.

[ketch]

Yeah, sorry, was being lazy when comparing the old code to the new. I will be removed.

That solution seems a bit extreme (throwing out the baby with the bathwater and all that...)

;-)

I originally was thinking along these lines but then thought it would be better to do the option that leaves the most control and least duplication of code available. We could provide convenience routines to do this. One thing of note, since both qbc and auxbc are passed to the custom routines it is possible just to set both and not worry about having two functions. Also a lambda function could be used to even better effect.

Okay, fair enough. I'm +1 for merging once the commented code is cleaned out.

[mandli]

Any ideas on how to do introspection on the passed variable names so I can detect whether the aux or q arrays have been passed? I am trying to avoid using an extra argument (although that's the easiest path forward).

[ketch]

A really quick and dirty approach:

try:
    method[2](state, dim, state.t, self.qbc, self.auxbc, self.num_ghost)
except TypeError:
    warn('You are using the old BC API which is deprecated and will fail in Clawpack 6.')
    if i==1:
        method[2](state, dim, state.t, self.qbc, self.num_ghost)
    else:
        method[2](state, dim, state.t, self.auxbc, self.num_ghost)

The TypeError gets thrown if you pass the wrong number of arguments. It's not pretty, but we're going to remove it eventually.

I will understand if you slap my hand for this.

[mandli]

That was what I was thinking for that problem actually. The problem I am thinking of is how I could determine which array was passed to one of the _bc_upper or _bc_lower functions so that I can apply the wall boundary conditions for the aux array properly. I could pass through the name of the array being set but that felt clunky.

[mandli]

I added something along the lines @ketch suggested and added a tidbit in the docs to describe the change clawpack/doc/pull/62.

[coveralls]

Coverage decreased (-0.57%) when pulling 5847713 on mandli:pass-aux-2-bc into a94b1a6 on clawpack:master.

[coveralls]

Coverage decreased (-0.59%) when pulling a882329 on mandli:pass-aux-2-bc into a94b1a6 on clawpack:master.