Fix compile_fn bug and reduce return type confusion

[michaelosthege]

This takes the bugfix from #5865 by @dfm albeit without the test case.

I looked into the test case and couldn't quite figure out why it's not working.
But if we were to test these module-level functions that are just forwarding to the Model methods, we should be doing that with mocks..

While I was at it, I also deprecated Model.compile_fn(point_fn=True) in favor of having Model.compile_fn vs. Model.compile_point_fn.
The motivation here is that switching the return type via a kwarg is really terrible from a typing perspective.

The Model.compile_point_fn() -> PointFunc signature on the other hand is less confusing.

Incompatible Changes

• None

Compatible Changes

• Model.compile_fn(point_fn=True) was deprecated in favor of a new Model.compile_point_fn() method

Bugfixes

• pm.compile_fn now correctly forwards the mode kwarg (closes Bug in pm.compile_fn #5867)
• Docstring formatting
• Type hints

[codecov]

Codecov Report

Merging #5909 (430aaa0) into main (0ba47b5) will decrease coverage by 0.01%.
The diff coverage is 80.95%.

❗ Current head 430aaa0 differs from pull request most recent head 7345677. Consider uploading reports for the commit 7345677 to get more accurate results

@@            Coverage Diff             @@
##             main    #5909      +/-   ##
==========================================
- Coverage   89.48%   89.46%   -0.02%     
==========================================
  Files          73       73              
  Lines       13275    13282       +7     
==========================================
+ Hits        11879    11883       +4     
- Misses       1396     1399       +3     

Impacted Files	Coverage Δ
pymc/backends/arviz.py	90.28% <ø> (ø)
pymc/model.py	87.38% <66.66%> (-0.49%)	⬇️
pymc/aesaraf.py	92.05% <100.00%> (+0.09%)	⬆️
pymc/backends/base.py	87.17% <100.00%> (ø)
pymc/step_methods/metropolis.py	83.36% <100.00%> (ø)
pymc/tuning/scaling.py	59.25% <100.00%> (ø)
pymc/tuning/starting.py	92.56% <100.00%> (ø)
pymc/step_methods/hmc/base_hmc.py	90.47% <0.00%> (+0.79%)	⬆️

[ricardoV94]

I don't think the point_fn distinction is useful enough to warrant separate methods, and specially a more verbose default one.

I understand the typing concerns, but then again... this is Python.

[michaelosthege]

Do we need the PointFunc at all? I mean it's just a matter of f(params) vs. f(**params) , right?

I'd really prefer to reduce the type confusion, because this function is used in a lot of places. Having well defined types here will help typing throughout the entire codebase.

[ricardoV94]

Yeah let's get rid of the silly wrapper. How would we go about doing it?

[michaelosthege]

I moved PointFunc to aesaraf.py.

I also looked into removing PointFunc alltogether, but this would alter the return type of compile_logp, compiled_dlogp and compile_d2logp.
So I'd leave it like this..

[ricardoV94]

Regardless of what we decide we still need to have a test that calls these functions. The fact that the original PR test failed makes this even more important.

[michaelosthege]

@ricardoV94 I neither have the time to start refactoring here, nor to figure out why the test Dan wrote ran into that error. I looked into it this morning, but couldn't figure out why.

The only reason why I picked up the issue is that this is a blocker for 4.0.1 which I want to get released ASAP.
I see no reason to delay this fix, or to start a big refactoring at this time.

[ricardoV94]

Then I suggest you limit the PR to the bugfix alone (and test)

[michaelosthege]

That original test failed not because of the forwarding of the Mode kwarg. IMO we should instead just remove those module-level functions and stick with the methods that we're using anyway.

But it's Monday again and I have other priorities. I can't justify investing more time here.
If there's anybody who can get this test working without dumping an hour of valuable time it's you, @ricardoV94. I'll be happy to review that and learn.

[michaelosthege]

Thanks for adding the test! Now that I see the solution it does make sense that inputs need to be specified..

Let's merge when the test go green 👍