Deprecate VoronoiFVM.Physics callbacks without data argument

[pjaap]

This patch implements a warning that is triggered if the user constructs a VoronoiFVM.Physics object with a callback function that uses only three or two arguments, respectively.

All examples were overhauled to comply with the new interface. Hereby, the data=nothing default has to be set to allow calls with and without the data argument. This needs to be done to preserve the current checks for isdata() and hasdata().
The default can be removed once the deprecated code is removed.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 87.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.78%. Comparing base (b176ec7) to head (c8377f4).
Report is 99 commits behind head on master.

Files with missing lines	Patch %	Lines
src/vfvm_physics.jl	86.36%	3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #122      +/-   ##
==========================================
- Coverage   85.33%   83.78%   -1.56%     
==========================================
  Files          24       24              
  Lines        2803     2911     +108     
==========================================
+ Hits         2392     2439      +47     
- Misses        411      472      +61     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[j-fu]

Was not aware of applicable...
I think data=nothing in all the examples will puzzle users. So may be we still can find another way how to trigger the deprecation warning.

[j-fu]

BTW the failure on nightly is due to some problems with RecursiveFactorizations.jl.

[j-fu]

May be instead of testing for !applicable(f,nn,nn,nn,nn) we can test for applicable(f,nn,nn,nn) ?

Also this works only if the callback functions are completely generic.
Im case a user has callbacks with some typed parameters, the proposed version would create false negatives which may be tolerable, while the implemented one would create false positives.

[pjaap]

Was not aware of applicable... I think data=nothing in all the examples will puzzle users. So may be we still can find another way how to trigger the deprecation warning.

I second that. The nothing default is only there to have a working solution. Another possibility is to provide data = [] to the physics object such that the isdata cases are fulfilled and callbacks with explicit data are done. Would you prefer that?

[pjaap]

May be instead of testing for !applicable(f,nn,nn,nn,nn) we can test for applicable(f,nn,nn,nn) ?

Also this works only if the callback functions are completely generic. Im case a user has callbacks with some typed parameters, the proposed version would create false negatives which may be tolerable, while the implemented one would create false positives.

Indeed, this is a better way. I will change that.

[pjaap]

After some thinking I had another idea. Instead of testing the signature of the callback functions, we directly check with applicable whether call with a data argument are possible.

• This simplifies the examples.
• I don't know how much this affects the speed of the code. I imagine that this check is more or less static and should be of the same cost as the isdata call. However, investigation should be done here.
• I am not sure where to put the deprecation warning. As it is now, it spams the warning very often in the assembly.
• There are some failing checks that needs to be investigated

[j-fu]

The simplified examples are good. However the test is not static, as the assembly code calls the fwrap, so the check is done all the time, and the warning is called as well.

I had the idea to use

@kwdef mutable struct NoData
  isdata=false
end

and define

isdata(d::NoData)=d.isdata
isdata(::Any)=true

After the test with applicable, the isdata flag in NoData could be overwritten.

[pjaap]

Yes, the applicable call within the wrapped function is not optimal. Where do you want to put the call?

The name of the proposed helper struct NoData should be MaybeData since it can represent data or not.

[j-fu]

MaybeData is good. I think the applicable call should remain in the constructor of Physics.

[j-fu]

As for the failing tests:

• on nightly, this is because of RecursiveFactorizations.jl. If this persists I would open an issue there
• I never really fixed codecov. I am not sure if it "sees" all the example tests. Should be fixed in v2 though.

[pjaap]

Ok, I think I have a working solution with MaybeData and the heuristic for the deprecation in the constructor.

• For some tests, e.g., Example125, the warning is still triggered. I am not sure why.
• With the changed code we cannot call boundary_dirichlet!(args...), since there is no method which accepts a data argument. Open an issue?

[j-fu]

• In Example125, the warning comes from the testfunction system.
• boundary_dirichlet!(system, ...) is at the edge of deprecation. It is better to specify this in the bconditon aka breaction callback.

[pjaap]

All warnings are now fixed, as far as I saw on my local machine. A changelog entry is also added.

[pjaap]

From my side I am now happy with the PR. I will squash this into less commits once it is approved.

[j-fu]

I am happy as well - good to go!

[pjaap]

Ok, squashed into 2 commits. There was a double entry in the changelog. I fixed that in the second commit.