Remove duplicate methods #6291

wd60622 · 2022-11-11T17:20:43Z

What is this PR about?
Noticed some duplicated methods in the pymc/model.py file mentioned in issue here.
Removed the duplication and tried to improve the docstrings

Checklist

Explain important implementation details 👆
Make sure that the pre-commit linting/style checks pass.
Link relevant issues (preferably in nice commit messages)
Are the changes covered by tests and docstrings?
Fill out the short summary sections 👇

Major / Breaking Changes

Using keyword seed for initial point no longer supported

Bugfixes / New features

NA

Docs / Maintenance

More consistent seeding argument with random_seed
Removed duplication function and method names throughout the package

wd60622 · 2022-11-11T17:24:48Z

pymc/model.py

+        random_seed : SeedSequenceSeed, default None
+            Seed(s) for generating initial point from the model. Used in pymc.aesaraf.reseed_rngs


Not sure if this is the correct indication of this type in the docstring. Kind of long created type. i.e.

pymc/pymc/aesaraf.py

Line 1021 in 5d7283e

SeedSequenceSeed = Optional[Union[int, Sequence[int], np.ndarray, np.random.SeedSequence]]

Also mentioned the function that this value is fed into. Not sure how that renders atm.

Looks about right (from here)

codecov · 2022-11-11T17:40:42Z

Codecov Report

Merging #6291 (9c74115) into main (5d7283e) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6291      +/-   ##
==========================================
+ Coverage   94.12%   94.14%   +0.02%     
==========================================
  Files         111      111              
  Lines       23861    23847      -14     
==========================================
- Hits        22458    22450       -8     
+ Misses       1403     1397       -6

Impacted Files	Coverage Δ
pymc/model.py	`90.32% <100.00%> (+0.63%)`	⬆️
pymc/smc/kernels.py	`97.37% <100.00%> (ø)`

ricardoV94 · 2022-11-11T19:29:56Z

pymc/model.py

@@ -1259,34 +1265,6 @@ def set_data(

        shared_object.set_value(values)

-    def initial_point(self, seed=None) -> Dict[str, np.ndarray]:


I think I probably caused this when trying to reorder the methods a bit more logically. There is a shape_from_dims below the initial point related methods which makes more sense above, close to the other coord/dims related methods.

I would therefore delete the repeated methods above and not these below.

Alright. I will reorder

I put after the dim/coords methods but before set_data. Thought that made enough sense

ricardoV94 · 2022-11-12T21:42:35Z

pymc/model.py

@@ -1137,6 +1101,48 @@ def set_dim(self, name: str, new_length: int, coord_values: Optional[Sequence] =
        self.dim_lengths[name].set_value(new_length)
        return

+    @property
+    def test_point(self) -> Dict[str, np.ndarray]:


We can get rid of this deprecated property as well

michaelosthege

Looks like this is ready to merge, right @ricardoV94 ?

michaelosthege · 2022-11-14T16:59:05Z

pymc/model.py

+        random_seed : SeedSequenceSeed, default None
+            Seed(s) for generating initial point from the model. Used in pymc.aesaraf.reseed_rngs


Looks about right (from here)

* remove dups, add type hints, and docstrings * replacing references to old code * reorder and reword * deprecate the "test_point" method * fix NameError. got to work locally

bmorris3 · 2023-01-11T00:20:54Z

Was it intentional/necessary to change the argument seed to random_seed? This leads to lots of breaks in things downstream.

michaelosthege · 2023-01-11T00:34:34Z

it was an intentional change, but next time we should go for a backwards compatible approach with a deprecation warning..

wd60622 · 2023-01-11T04:31:41Z

Was it intentional/necessary to change the argument seed to random_seed? This leads to lots of breaks in things downstream.

Can you be specific with "lots" of breaking changes?

Is there something that isn't covered in the tests? Or were your solutions just a grep replace? Replacements in internal code or only personal written sections?

wd60622 added 2 commits November 11, 2022 12:10

remove dups, add type hints, and docstrings

45e4107

replacing references to old code

b2adb39

wd60622 commented Nov 11, 2022

View reviewed changes

ricardoV94 reviewed Nov 11, 2022

View reviewed changes

reorder and reword

8b4330f

ricardoV94 reviewed Nov 12, 2022

View reviewed changes

wd60622 added 2 commits November 13, 2022 13:24

deprecate the "test_point" method

e52f8f4

fix NameError. got to work locally

9c74115

michaelosthege approved these changes Nov 14, 2022

View reviewed changes

michaelosthege added maintenance major Include in major changes release notes section labels Nov 14, 2022

ricardoV94 approved these changes Nov 14, 2022

View reviewed changes

michaelosthege merged commit 24888ac into pymc-devs:main Nov 14, 2022

wd60622 deleted the duplicate_initial_point_method branch November 14, 2022 19:37

tomicapretto mentioned this pull request Nov 23, 2022

Add common numpy transforms to extra_namespace bambinos/bambi#589

Merged

bmorris3 mentioned this pull request Jan 11, 2023

Bugfix for eval_in_model exoplanet-dev/pymc-ext#37

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove duplicate methods #6291

Remove duplicate methods #6291

wd60622 commented Nov 11, 2022 •

edited

Loading

wd60622 Nov 11, 2022

michaelosthege Nov 14, 2022

codecov bot commented Nov 11, 2022 •

edited

Loading

ricardoV94 Nov 11, 2022 •

edited

Loading

wd60622 Nov 11, 2022

wd60622 Nov 12, 2022

ricardoV94 Nov 12, 2022

wd60622 Nov 13, 2022

michaelosthege left a comment

michaelosthege Nov 14, 2022

bmorris3 commented Jan 11, 2023

michaelosthege commented Jan 11, 2023

wd60622 commented Jan 11, 2023 •

edited

Loading

		random_seed : SeedSequenceSeed, default None
		Seed(s) for generating initial point from the model. Used in pymc.aesaraf.reseed_rngs

		@@ -1259,34 +1265,6 @@ def set_data(

		shared_object.set_value(values)

		def initial_point(self, seed=None) -> Dict[str, np.ndarray]:

Remove duplicate methods #6291

Remove duplicate methods #6291

Conversation

wd60622 commented Nov 11, 2022 • edited Loading

Major / Breaking Changes

Bugfixes / New features

Docs / Maintenance

wd60622 Nov 11, 2022

Choose a reason for hiding this comment

michaelosthege Nov 14, 2022

Choose a reason for hiding this comment

codecov bot commented Nov 11, 2022 • edited Loading

Codecov Report

ricardoV94 Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

wd60622 Nov 11, 2022

Choose a reason for hiding this comment

wd60622 Nov 12, 2022

Choose a reason for hiding this comment

ricardoV94 Nov 12, 2022

Choose a reason for hiding this comment

wd60622 Nov 13, 2022

Choose a reason for hiding this comment

michaelosthege left a comment

Choose a reason for hiding this comment

michaelosthege Nov 14, 2022

Choose a reason for hiding this comment

bmorris3 commented Jan 11, 2023

michaelosthege commented Jan 11, 2023

wd60622 commented Jan 11, 2023 • edited Loading

wd60622 commented Nov 11, 2022 •

edited

Loading

codecov bot commented Nov 11, 2022 •

edited

Loading

ricardoV94 Nov 11, 2022 •

edited

Loading

wd60622 commented Jan 11, 2023 •

edited

Loading