Fix 'model' and 'aesara_config' kwargs for pm.Model

[ferrine]

Fixes

with pm.Model(aesara_config=dict(mode="JAX")):
    ...

and

with pm.Model(model=parent):
    ...

[ricardoV94]

I don't think we are using that anywhere...? All functions that end up compiling stuff accept mode

[ricardoV94]

I don't think we are using that anywhere...? All functions that end up compiling stuff accept mode

Meaning we should just remove it if it is indeed never used.

[ferrine]

I don't think we are using that anywhere...? All functions that end up compiling stuff accept mode

Meaning we should just remove it if it is indeed never used.

I tried to run VI using Jax and failed to do so (couple of more issues to be solved), I hoped this method would work for me as it was implemented in v3.

[ricardoV94]

I just know it was removed because of the release notes. I don't know how it was used before and I don't remember seeing it being used anywhere in the library since I started working on it.

You can of course use JAX or NUMBA by setting the global aesara compile flag or passing mode="JAX" to pm.sample or whichever method you are using.

[codecov]

Codecov Report

Merging #5915 (5dfe789) into main (45a816e) will decrease coverage by 7.66%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5915      +/-   ##
==========================================
- Coverage   89.50%   81.83%   -7.67%     
==========================================
  Files          73       73              
  Lines       13276    13275       -1     
==========================================
- Hits        11883    10864    -1019     
- Misses       1393     2411    +1018     

Impacted Files	Coverage Δ
pymc/model.py	88.07% <100.00%> (+0.01%)	⬆️
pymc/smc/smc.py	18.89% <0.00%> (-77.56%)	⬇️
pymc/smc/sample_smc.py	16.77% <0.00%> (-66.45%)	⬇️
pymc/distributions/bound.py	45.54% <0.00%> (-54.46%)	⬇️
pymc/step_methods/mlda.py	50.41% <0.00%> (-45.97%)	⬇️
pymc/distributions/simulator.py	46.52% <0.00%> (-40.98%)	⬇️
pymc/step_methods/metropolis.py	50.93% <0.00%> (-32.44%)	⬇️
pymc/distributions/censored.py	76.74% <0.00%> (-16.28%)	⬇️
pymc/sampling.py	70.02% <0.00%> (-12.39%)	⬇️
pymc/distributions/discrete.py	86.97% <0.00%> (-12.24%)	⬇️
... and 14 more

[ricardoV94]

I was referring to this line:

pymc/RELEASE-NOTES.md

Line 125 in 121a5ae

- `Model(theano_config=...)` kwarg was removed

But maybe it was just renamed, not removed? I see it is used in the ContextMeta

[ferrine]

I was referring to this line:

pymc/RELEASE-NOTES.md

Line 125 in 121a5ae

- `Model(theano_config=...)` kwarg was removed

But maybe it was just renamed, not removed? I see it is used in the ContextMeta

I failed to find an issue about this change. Neither removing model nor aesara/theano_config.

[ferrine]

Oh, another query gave this: #4981

[ricardoV94]

Oh, another query gave this: #4981

Do you think that was a misunderstanding (ignoring the __new__ path)? CC @michaelosthege

[ferrine]

Oh, another query gave this: #4981

Do you think that was a misunderstanding (ignoring the new path)? CC @michaelosthege

The arguments might seem to be unused, but they are passed to __new__, if missing in __init__ you get errors

[ricardoV94]

Oh, another query gave this: #4981

Do you think that was a misunderstanding (ignoring the new path)? CC @michaelosthege

The arguments might seem to be unused, but they are passed to __new__, if missing in __init__ you get errors

Yeah, it seems to be the case, and your new tests clearly show it has an effect.

[michaelosthege]

Is __new__ automatically called before __init__???

The theano_config kwarg was intentionally removed without replacement. It looks like it was forgotten to make changes in __new__.

We should

• either re-introduce the kwargs in __init__ while raising a TypeError with instructions to use Aesara config functions directly (like we do in tests)
• XOR remove it also from __new__

[ferrine]

Is __new__ automatically called before __init__???

Yes, it is called before __init__. I'm fine to remove aesara_config, and keep the model kwarg

[ricardoV94]

Is __new__ automatically called before __init__???

Yes, it is called before __init__. I'm fine to remove aesara_config, and keep the model kwarg

+1 for less magic stuff behind the scenes

[michaelosthege]

I'm not a fan of keeping an API that is seldom used, but it looks like the removal was incomplete and this PR fixes it.

At some point we might want to remove aesara_config entirely, but that's out of scope for now.

So let's merge this in order to get 4.1.0 over the finish line?