Deprecate remaining VI methods #5642

purna135 · 2022-03-22T13:06:19Z

Addressing #5581

Replaced remaining NotImplementedInference methods with deprecation warning.

codecov · 2022-03-22T13:25:17Z

Codecov Report

Merging #5642 (4623916) into main (b5a5b56) will decrease coverage by 7.32%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##             main    #5642      +/-   ##
==========================================
- Coverage   89.36%   82.04%   -7.33%     
==========================================
  Files          74       73       -1     
  Lines       13805    13152     -653     
==========================================
- Hits        12337    10790    -1547     
- Misses       1468     2362     +894

Impacted Files	Coverage Δ
pymc/sampling_jax.py	`0.00% <0.00%> (-96.97%)`	⬇️
pymc/tuning/starting.py	`92.56% <ø> (-0.13%)`	⬇️
pymc/variational/__init__.py	`100.00% <ø> (ø)`
pymc/variational/opvi.py	`36.80% <15.00%> (-45.47%)`	⬇️
pymc/variational/approximations.py	`43.72% <25.00%> (-22.35%)`	⬇️
pymc/variational/inference.py	`18.94% <33.33%> (-68.30%)`	⬇️
pymc/parallel_sampling.py	`86.79% <66.66%> (+0.08%)`	⬆️
pymc/sampling.py	`70.21% <76.74%> (-18.22%)`	⬇️
pymc/aesaraf.py	`90.88% <100.00%> (-0.74%)`	⬇️
pymc/distributions/censored.py	`93.02% <100.00%> (-1.72%)`	⬇️
... and 30 more

purna135 · 2022-03-22T17:00:48Z

I need some help with the next step; is adding a deprecation warning the right way to go?

ghost · 2022-03-23T01:09:59Z

I think the idea was to just remove them (even though the title in the issue says "deprecate"). The methods are not implemented in v4, so there is really nothing to deprecate.

purna135 · 2022-03-26T18:11:35Z

Could someone please help me remove the Normalizing flows?
I'm not sure if I should remove the entire NormalizingFlowGroup class or just a few lines of code.

pymc/pymc/variational/approximations.py

Lines 362 to 364 in ef5f91b

    
           def __init_group__(self, group): 
        
               raise NotImplementedInference("Normalizing flows are not yet ported to v4") 
        
               super().__init_group__(group)

canyon289 · 2022-04-17T23:34:34Z

@ferrine would you like to take a look here?

ferrine

As for normalizing flows I expect something like rm pymc/variational/flows.py as a good first step

ferrine · 2022-04-22T16:39:54Z

pymc/variational/approximations.py

@@ -270,14 +272,14 @@ def _new_initial(self, size, deterministic, more_replacements=None):
                at.repeat(self.mean.reshape((1, -1)), size, -1),
                self.histogram[self.randidx(size)],
            )
+        elif deterministic:
+            warnings.warn(
+                "Deterministic sampling from a Histogram is deprecated and will be removed soon.",


Is it indeed broken or it was a false alarm?

We can enable the test for that and check

ferrine · 2022-04-22T16:41:12Z

pymc/variational/opvi.py

@@ -957,9 +957,9 @@ def __init_group__(self, group):
        if not group:
            raise GroupError("Got empty group")
        if self.local:


I think the context of the issue was to entirely delete the code and local kwarg, feel free to just delete large portions of the code.

ferrine · 2022-04-22T16:41:43Z

pymc/variational/opvi.py

@@ -957,9 +957,9 @@ def __init_group__(self, group):
        if not group:
            raise GroupError("Got empty group")
        if self.local:
-            raise NotImplementedInference("Local inferene aka AEVB is not supported in v4")
+            warnings.warn("Local inferene aka AEVB is deprecated.", DeprecationWarning)
        if self.batched:


Same for batched kwarg and all its mentions. Just delete without hesitation

fonnesbeck · 2022-04-23T19:23:48Z

Is this enough to go on @purna135 ?

purna135 · 2022-04-23T19:28:56Z

I'll work on these suggestions and let you know if I need anything else.
Thank you very much.

michaelosthege · 2022-05-21T19:52:47Z

Hi @purna135, this PR is one of the final steps towards v4.0.0. Could you pick it up again and push it over the finish line?

purna135 · 2022-05-21T19:59:33Z

Hi @michaelosthege sorry for the delay; I was preoccupied with my exam, which has now been done.
I will complete this RP in few days.

twiecki · 2022-05-22T11:12:37Z

pymc/variational/opvi.py

@@ -957,9 +957,9 @@ def __init_group__(self, group):
        if not group:
            raise GroupError("Got empty group")
        if self.local:
-            raise NotImplementedInference("Local inferene aka AEVB is not supported in v4")
+            warnings.warn("Local inferene aka AEVB is deprecated.", DeprecationWarning)


We should just delete this code, rather than deprecate.

purna135 · 2022-05-23T11:21:41Z

Hi @twiecki and @ferrine could you please have a look if this is the right way to go?

ferrine

Looks good to me, can be merged once tests pass

purna135 · 2022-05-24T11:34:56Z

Thank you very much, @ferrine. Could you kindly advise me on how to pass these failed tests?
In the doc string, I realised I left some references to batched and local, which I will remove in my next commit.

ferrine · 2022-05-24T11:59:37Z

@purna135, the tests diff was collapsed. I think I missed something here. I have to go through it once more

ferrine · 2022-05-24T13:06:18Z

pymc/tests/test_variational_inference.py

-        (not_raises(), {MeanFieldGroup: ["one"], FullRankGroup: ["two", "three"]}),
-    ],
-)
-@ignore_not_implemented_inference


the ignore_not_implemented_inference hook was used to ignore tests that fail due to the not supported functionality.

NotImplementedInference is an explicit error not to confuse failures with other "bugs". Most of the tests that are deleted so far are valid tests.

The commit that deletes all these tests should be reverted. Or, if not easy, you can use the original file that is present in master.

To summarise.

The only obsolete thing in the test file is ignore_not_implemented_inference.

Once it is removed, all tests should pass and no NotImplementedInference is raised.

The only tests to remove are that fail because of the removed functionality.

ferrine

I missed the collapsed diff of tests. Most of the deleted tests are valid. I've left a comment above with the explanation.

ferrine · 2022-05-24T14:26:41Z

We did a quick video chat with @purna135 to avoid further confusion. Additional thing to revisit was the documentation. That missed my eye as well.

purna135 · 2022-05-24T14:30:53Z

Thank you so much for your time and help @ferrine : )

purna135 · 2022-05-25T18:34:01Z

Hello, @ferrine. I deleted several flow-related tests and refactored others, but there are still 10 tests that I am unable to refactor and have commented so that other tests can be checked. Could you please assist me in refactoring the tests I commented?
Many thanks

twiecki marked this pull request as draft March 22, 2022 13:21

twiecki changed the title ~~[WIP] Deprecate remaining VI methods~~ Deprecate remaining VI methods Mar 22, 2022

ferrine reviewed Apr 22, 2022

View reviewed changes

michaelosthege added this to the v4.0.0 milestone May 21, 2022

twiecki reviewed May 22, 2022

View reviewed changes

purna135 added 2 commits May 22, 2022 17:54

Deprecated remaining VI methods

55af8f6

Deleted flows and code for local and batched kwarg

66c1786

purna135 force-pushed the deprecate_vi_methods branch from 473e28e to 66c1786 Compare May 23, 2022 10:37

twiecki requested a review from ferrine May 24, 2022 10:48

ferrine approved these changes May 24, 2022

View reviewed changes

ferrine reviewed May 24, 2022

View reviewed changes

ferrine requested changes May 24, 2022

View reviewed changes

purna135 added 4 commits May 24, 2022 23:41

Merge remote-tracking branch 'upstream/main' into deprecate_vi_methods

c693254

removed rowwise from opvi.py

f25543e

redo the test_variational_inference.py

9c8d265

refactored some test cases

ddd68f9

purna135 added 2 commits May 25, 2022 23:37

removed mention of local from docstring

fa2fbb3

resolved merge conflict

28e697a

corrected 'MeanFieldGroup' object has no attribute 'rng'

4623916

purna135 closed this May 25, 2022

purna135 deleted the deprecate_vi_methods branch May 25, 2022 19:02

purna135 restored the deprecate_vi_methods branch May 25, 2022 19:03

purna135 reopened this May 25, 2022

purna135 closed this May 25, 2022

purna135 deleted the deprecate_vi_methods branch May 25, 2022 19:24

purna135 restored the deprecate_vi_methods branch May 25, 2022 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate remaining VI methods #5642

Deprecate remaining VI methods #5642

purna135 commented Mar 22, 2022

codecov bot commented Mar 22, 2022 •

edited

Loading

purna135 commented Mar 22, 2022

ghost commented Mar 23, 2022

purna135 commented Mar 26, 2022 •

edited

Loading

canyon289 commented Apr 17, 2022

ferrine left a comment

ferrine Apr 22, 2022

ferrine Apr 22, 2022

ferrine Apr 22, 2022

ferrine Apr 22, 2022 •

edited

Loading

fonnesbeck commented Apr 23, 2022

purna135 commented Apr 23, 2022

michaelosthege commented May 21, 2022

purna135 commented May 21, 2022

twiecki May 22, 2022

purna135 commented May 23, 2022

ferrine left a comment

purna135 commented May 24, 2022

ferrine commented May 24, 2022

ferrine May 24, 2022 •

edited

Loading

ferrine left a comment •

edited

Loading

ferrine commented May 24, 2022

purna135 commented May 24, 2022

purna135 commented May 25, 2022

Deprecate remaining VI methods #5642

Deprecate remaining VI methods #5642

Conversation

purna135 commented Mar 22, 2022

codecov bot commented Mar 22, 2022 • edited Loading

Codecov Report

purna135 commented Mar 22, 2022

ghost commented Mar 23, 2022

purna135 commented Mar 26, 2022 • edited Loading

canyon289 commented Apr 17, 2022

ferrine left a comment

Choose a reason for hiding this comment

ferrine Apr 22, 2022

Choose a reason for hiding this comment

ferrine Apr 22, 2022

Choose a reason for hiding this comment

ferrine Apr 22, 2022

Choose a reason for hiding this comment

ferrine Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

fonnesbeck commented Apr 23, 2022

purna135 commented Apr 23, 2022

michaelosthege commented May 21, 2022

purna135 commented May 21, 2022

twiecki May 22, 2022

Choose a reason for hiding this comment

purna135 commented May 23, 2022

ferrine left a comment

Choose a reason for hiding this comment

purna135 commented May 24, 2022

ferrine commented May 24, 2022

ferrine May 24, 2022 • edited Loading

Choose a reason for hiding this comment

ferrine left a comment • edited Loading

Choose a reason for hiding this comment

ferrine commented May 24, 2022

purna135 commented May 24, 2022

purna135 commented May 25, 2022

codecov bot commented Mar 22, 2022 •

edited

Loading

purna135 commented Mar 26, 2022 •

edited

Loading

ferrine Apr 22, 2022 •

edited

Loading

ferrine May 24, 2022 •

edited

Loading

ferrine left a comment •

edited

Loading