tfluct as input to solver, following rp

[sanromd]

This PR

• adds the ability to provide a tfluct solver to SharpClaw directly in the same fashion as Riemann solvers are passed. When solver.tfluct_solver=True, a fortran python-wrapped routine can be given through import _name-of-tfluct-solver_, solver.tfluct=_name-of-tfluct-solver_
• solver follows API convention of Riemann solvers
• works in n-d
• euler_1d has been modified to provide an example of usage

[sanromd]

@ketch @hadjimy this PR adds the ability we have been discussing. Likely, it could be beneficial to do the same for evec, maybe by setting solver.evec_decomp=True/False and provide the evec routine through solver.evec

[mandli]

Is this stub supposed to match the example above? The function signatures and name are different.

[sanromd]

@mandli the tfluct stub example was left untouched for a long time, the API was never updated. This PR fixes that and provides a method to set tfluct following this standard, see for example euler_1d

[mandli]

I think I see what you are saying although now having different signatures for each dimension's implementation (which I think is fine) means you may want to have the stub mimic this (i.e. have stub for tfluct1, tfluct2, and tfluct3 which have the correct signatures). Often times users will copy these stub functions and use them so this could be dangerous otherwise even if you are passing them through to flux*.

[sanromd]

@mandli following your suggestion I have added dimension dependent stub functions for tfluct.

[sanromd]

@ketch any comments?

[sanromd]

@mandli note that now the stub functions are different for 1d and 2d...

[ketch]

Please delete these stubs and write some documentation of the interface instead.

[mandli]

At this point I defer approval to those that know more about SharpClaw.

[sanromd]

@ahmadia @ketch

[ketch]

The correct name for this variable here is adq, not amdq2. The function is supposed to compute the total fluctuation.

[ketch]

This PR is a model of good practice: it does one thing only, and what it does is explained clearly.

[ahmadia]

Hi :)

Agreed that this is a good PR.

Comments:

• This introduces some complicated functionality. It would be nice if you added two small tests, one that correctly imports a tfluct solver and runs for a very short time, and another that raises an error when trying to set tfluct but without importing an appropriate module
• I'm not familiar with what tfluct is or how it is used. Perhaps it would be worth adding just a couple of sentences to the PyClaw documentation?
• Agreed with David's statement earlier, you also need to sort out whether the tfluct module will be available by default (compiled into the SharpClaw Fortran extension module) or whether it will only be available in examples. It should not be in both. I lean towards making things available in libraries when we install Clawpack, but I don't have a strong opinion.

[ketch]

Regarding the stub functions: I think you can get rid of them completely. Instead, just put the following near the top of sharpclaw/solver.py:

def empty_fun():
    raise Exception('This is a dummy function and should never be called.')

Then

if solver.tfluct_solver == False:
    solver.tfluct = empty_fun

[ketch]

I think that eventually it would make sense to keep tfluct solvers in the Riemann repository and handle them just like Riemann solvers. However, since we only have one tfluct solver at present, it doesn't seem worthwhile to disrupt the current layout of the Riemann repository. And it is useful to have an example showing how a user can employ their own tfluct solver.

[mandli]

A couple of comments about what @ketch said:

• Wouldn't the stub functions have to be present if they are being called from Fortran (some platforms, notably those with the ability to have a non-flat namespace, should be fine)? I guess you are thinking that we would handle this as a cpointer into the Fortran? I might not be thinking about this quite right so please correct me if I am off base on this point.
• If what you suggest would work then I think it should also be said that the stub functions also provide a nice, empty template for those who need to write their own. Beyond this small quibble I think removing the stub functions more broadly is not a bad idea.
• I fully support moving the tfluct solvers into the Riemann repository as soon as it makes sense to do so.

[ketch]

Response to @mandli :

Wouldn't the stub functions have to be present if they are being called from Fortran (some platforms, notably those with the ability to have a non-flat namespace, should be fine)? I guess you are thinking that we would handle this as a cpointer into the Fortran? I might not be thinking about this quite right so please correct me if I am off base on this point.

The functions don't get called from the fortran unless tfluct_solver is True. The code I wrote in my previous comment works fine (I tested it).

If what you suggest would work then I think it should also be said that the stub functions also provide a nice, empty template for those who need to write their own. Beyond this small quibble I think removing the stub functions more broadly is not a bad idea.

True. But if those functions are really just documentation of an API, then we should instead document the API (in the docs!)

[mandli]

Response to @ketch (nested quoting works!):

Wouldn't the stub functions have to be present if they are being called from Fortran (some platforms, notably those with the ability to have a non-flat namespace, should be fine)? I guess you are thinking that we would handle this as a cpointer into the Fortran? I might not be thinking about this quite right so please correct me if I am off base on this point.

The functions don't get called from the fortran unless tfluct_solver is True. The code I wrote in my previous comment works fine (I tested it).

Yeah, now that I think about it as long as it is being passed as a function argument it should be fine. It might be worthwhile checking on Linux whose ELF binary format is more sensitive to this than OS X's Mach-O format.

If what you suggest would work then I think it should also be said that the stub functions also provide a nice, empty template for those who need to write their own. Beyond this small quibble I think removing the stub functions more broadly is not a bad idea.

True. But if those functions are really just documentation of an API, then we should instead document the API (in the docs!)

Heh, good point. I think I am too used to what we have needed to do in the past with Fortran.

[coveralls]

Coverage increased (+0.0%) when pulling c6ea3a6 on sanromd:dev-tfluct into d6c8f92 on clawpack:master.

[ketch]

Just to be clear: this PR is waiting on @sanromd to clean it up following the comments above.

[coveralls]

Coverage increased (+0.0%) when pulling c891e4b on sanromd:dev-tfluct into a94b1a6 on clawpack:master.

[coveralls]

Coverage increased (+0.0%) when pulling a36a7e5 on sanromd:dev-tfluct into a94b1a6 on clawpack:master.

[sanromd]

@ketch The issues raised have been solved. I have left the stub functions tfluct on sharp claw for reference to the user, although they are not used anymore. Perhaps we could create a dummy folder on Riemann for the tfluct solvers as a separate PR.

I think now this is ready to merge.

[coveralls]

Coverage decreased (-0.04%) when pulling b1a0d5a on sanromd:dev-tfluct into a94b1a6 on clawpack:master.

[coveralls]

Coverage decreased (-0.04%) when pulling b1a0d5a on sanromd:dev-tfluct into a94b1a6 on clawpack:master.

[sanromd]

@ketch corrections committed

[coveralls]

Coverage decreased (-1.93%) when pulling a1de1d3 on sanromd:dev-tfluct into a94b1a6 on clawpack:master.

[coveralls]

Coverage decreased (-0.02%) when pulling 06d57dd on sanromd:dev-tfluct into 1fef8a4 on clawpack:master.

[coveralls]

Coverage decreased (-0.02%) when pulling 06d57dd on sanromd:dev-tfluct into 1fef8a4 on clawpack:master.