Remove sd optional kwarg from distributions #5583

purna135 · 2022-03-11T17:51:51Z

Removed sd kwarg in favor of sigma from several continuous univariate distributions.

…ipts/run_mypy.py list

codecov · 2022-03-11T18:10:49Z

Codecov Report

Merging #5583 (c5fcc70) into main (2db28f0) will increase coverage by 0.06%.
The diff coverage is 80.95%.

@@            Coverage Diff             @@
##             main    #5583      +/-   ##
==========================================
+ Coverage   87.60%   87.66%   +0.06%     
==========================================
  Files          76       76              
  Lines       13745    13712      -33     
==========================================
- Hits        12041    12021      -20     
+ Misses       1704     1691      -13

Impacted Files	Coverage Δ
pymc/data.py	`83.61% <ø> (ø)`
pymc/model.py	`85.95% <ø> (ø)`
pymc/distributions/timeseries.py	`22.89% <33.33%> (+0.53%)`	⬆️
pymc/distributions/continuous.py	`98.07% <100.00%> (+0.84%)`	⬆️
pymc/distributions/mixture.py	`94.73% <100.00%> (+1.59%)`	⬆️
pymc/parallel_sampling.py	`86.71% <0.00%> (-1.00%)`	⬇️

purna135 · 2022-03-11T18:13:06Z

Should I replace sd with sigma in the test which are calling any function with sd arguments?

canyon289 · 2022-03-11T18:16:36Z

Should I replace sd with sigma in the test which are calling any function with sd arguments?

Yes please, totally eliminate it from the codebase

purna135 · 2022-03-11T18:24:12Z

totally eliminate it from the codebase

Do you mean, the complete test function which are using sd parameter ?

ricardoV94 · 2022-03-11T18:28:45Z

totally eliminate it from the codebase

Do you mean, the complete test function which are using sd parameter ?

No. Keep the function, just rename it to sigma as you first suggested.

purna135 · 2022-03-11T21:29:58Z

Hello, I'm having trouble understanding what these lines below are doing:

pymc/pymc/tests/test_sampling.py

Lines 737 to 739 in 5eaa516

    
           sigma_in_1 = pm.HalfNormal("sd_in_1", 1) 
        
           mu_in_2 = pm.Normal("mu_in_2", 0, 2) 
        
           sigma_in_2 = pm.HalfNormal("sd__in_2", 1)

pymc/pymc/tests/test_transforms.py

Lines 335 to 340 in 5eaa516

    
                   "sd,size", 
        
                   [ 
        
                       (2.5, 2), 
        
                       (5.0, (2, 3)), 
        
                       (np.ones(3) * 10.0, (4, 3)), 
        
                   ],

ricardoV94 · 2022-03-12T07:38:09Z

Hello, I'm having trouble understanding what these lines below are doing:

pymc/pymc/tests/test_sampling.py

Lines 737 to 739 in 5eaa516

sigma_in_1 = pm.HalfNormal("sd_in_1", 1)

mu_in_2 = pm.Normal("mu_in_2", 0, 2)

sigma_in_2 = pm.HalfNormal("sd__in_2", 1)

That one does not seem to involve the sd kwarg. Is it failing or are you asking because the variable has sd in the name? If it's passing you can leave as is.

pymc/pymc/tests/test_transforms.py

Lines 335 to 340 in 5eaa516

"sd,size",

[

(2.5, 2),

(5.0, (2, 3)),

(np.ones(3) * 10.0, (4, 3)),

],

That one you need to change that dict to havesigma as a key instead of sd. I guess in the function that's called the dictionary is unpacked as kwargs like Halfnormal(**{"sigma": 2})

purna135 · 2022-03-14T11:16:11Z

Hi @ricardoV94, @canyon289,

Running pytest takes an excessive amount of time.
I ran the pytest 24 hours ago and it's still at 98 percent.

Is it possible to do a quick test on local system before pushing to github to see if there are any errors?

ricardoV94 · 2022-03-14T11:59:52Z

Hi @ricardoV94, @canyon289,

Running pytest takes an excessive amount of time. I ran the pytest 24 hours ago and it's still at 98 percent.

Is it possible to do a quick test on local system before pushing to github to see if there are any errors?

You shouldn't try to run the entire test suite on your local machine. Usually only tests that you added or that you know are directly related to changes you've made.

purna135 · 2022-03-14T12:12:48Z

Hi @ricardoV94, I only fixed the errors I was getting the previous time, and it passed all of the tests this time.

However, I can still find some tests/methods/notebooks that use sd, for example:

pymc/pymc/tests/test_shape_handling.py

Lines 392 to 397 in 8cb71a5

    
           pm.Normal("size64", mu=0, sd=1, size=size64, observed=obs) 
        
           pm.Normal("shape64", mu=0, sd=1, shape=size64, observed=obs) 
        
           model.logp() 
        
           pm.Normal("size_cast64", mu=0, sd=1, size=cast64, observed=obs) 
        
           pm.Normal("shape_cast64", mu=0, sd=1, shape=cast64, observed=obs)

https://github.com/pymc-devs/pymc/blob/main/docs/source/learn/examples/dimensionality.ipynb

Should I rename them to sigma as well ?

ricardoV94 · 2022-03-14T21:28:42Z

Should I rename them to sigma as well ?

Yes 👍

… remaned it for consistency

…t_observed_with_column_vector

review-notebook-app · 2022-03-15T07:44:33Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

purna135 · 2022-03-15T08:04:20Z

Hello, @ricardoV94! I believe I am finished; please take a look.
How to resolve this conflicts ?

twiecki · 2022-03-15T09:24:01Z

Release notes

purna135 · 2022-03-15T20:08:20Z

Release notes

Yes, the release note mentions sd but I assumed it would be handled by the Member. Is it possible for me to make changes to that?

twiecki · 2022-03-16T09:17:07Z

Yes certainly.

…

On Tue, Mar 15, 2022, 21:08 Purna Chandra Mansingh ***@***.***> wrote: Release notes Yes, the release note mentions sd but I assumed it would be handled by the Member. Is it possible for me to make changes to that? — Reply to this email directly, view it on GitHub <#5583 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFETGEIT6SOIAWSI35B5MLVADU35ANCNFSM5QQI3ELQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

purna135 · 2022-03-16T12:52:11Z

Under the Deprecations header for PyMC vNext, I'll add a point like "In favour of sigma, the deprecated sd has been removed.", right?

twiecki · 2022-03-17T08:12:51Z

Yes, exactly.

purna135 · 2022-03-17T19:29:10Z

Done 😃, I believe it's finished now.

RELEASE-NOTES.md

purna135 · 2022-03-18T07:57:11Z

Is there anything I can do to resolve this conflict?

ricardoV94 · 2022-03-18T07:59:09Z

Is there anything I can do to resolve this conflict?

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line

ricardoV94 · 2022-03-18T11:02:06Z

Thanks @purna135!

purna135 · 2022-03-18T11:08:17Z

Thanks for this opportunity ☺️

purna135 added 4 commits March 11, 2022 16:34

Removed sd optional kwarg from continuous.py

0a65c1f

Removed sd optional kwarg from timeseries.py

29c0016

Removed sd optional kwarg from mixture.py

8acda64

removed pymc/sampling_jax.py and pymc/step_methods/sgmcmc.py from scr…

6b34983

…ipts/run_mypy.py list

OriolAbril mentioned this pull request Mar 12, 2022

Update pymc.TruncatedNormal docstring #5546

Merged

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

40c939c

sd renamed to sigma

33e5463

purna135 added 12 commits March 15, 2022 04:53

renamed sd to sigma in benchmarks.py

01b0ce0

though rename of sd to sigma is not required in test_mixture.py, just…

72e4915

… remaned it for consistency

renamed sd to sigma in test_shape_handling.py::TestShapeDimsSize::tes…

c9a71dc

…t_observed_with_column_vector

renamed sd to sigma in dimensionality.ipynb

7bef2d6

renamed sd to sigma in test_util.py

23a0f0a

renamed sd to sigma in tests/models.py

1e723bd

renamed sd to sigma in distributions.py

72186c4

renamed sd to sigma in data.py

d827e70

renamed sd to sigma in examples/posterior_predictive.ipynb

e14e799

renamed sd to sigma in developer_guide.rst

e761f50

renamed sd to sigma in source/PyMC_and_Aesara.rst

4c1a001

renamed sd to sigma in examples/GLM_linear.ipynb

853b0ff

renamed sd to sigma in pymc/model.py

8e2be8d

fixed examples/GLM_linear.ipynb

8ba28b5

twiecki approved these changes Mar 15, 2022

View reviewed changes

ricardoV94 changed the title ~~[WIP] Remove sd optional kwarg from distributions~~ Remove sd optional kwarg from distributions Mar 17, 2022

added a note "sd has been removed" in Release note

e45e65b

ricardoV94 reviewed Mar 18, 2022

View reviewed changes

RELEASE-NOTES.md Outdated Show resolved Hide resolved

✔ Updated deprecation note and added link to PR.

7d01461

ricardoV94 approved these changes Mar 18, 2022

View reviewed changes

resolved conflict in test_sampling.py

c5fcc70

ricardoV94 merged commit e8c07ef into pymc-devs:main Mar 18, 2022

purna135 deleted the remove_sd branch March 18, 2022 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove sd optional kwarg from distributions #5583

Remove sd optional kwarg from distributions #5583

purna135 commented Mar 11, 2022 •

edited by ricardoV94

Loading

codecov bot commented Mar 11, 2022 •

edited

Loading

purna135 commented Mar 11, 2022

canyon289 commented Mar 11, 2022

purna135 commented Mar 11, 2022

ricardoV94 commented Mar 11, 2022 •

edited

Loading

purna135 commented Mar 11, 2022

ricardoV94 commented Mar 12, 2022

purna135 commented Mar 14, 2022 •

edited

Loading

ricardoV94 commented Mar 14, 2022

purna135 commented Mar 14, 2022 •

edited

Loading

ricardoV94 commented Mar 14, 2022

review-notebook-app bot commented Mar 15, 2022

purna135 commented Mar 15, 2022

twiecki commented Mar 15, 2022

purna135 commented Mar 15, 2022

twiecki commented Mar 16, 2022 via email

purna135 commented Mar 16, 2022

twiecki commented Mar 17, 2022

purna135 commented Mar 17, 2022

purna135 commented Mar 18, 2022

ricardoV94 commented Mar 18, 2022

ricardoV94 commented Mar 18, 2022

purna135 commented Mar 18, 2022

Remove sd optional kwarg from distributions #5583

Remove sd optional kwarg from distributions #5583

Conversation

purna135 commented Mar 11, 2022 • edited by ricardoV94 Loading

codecov bot commented Mar 11, 2022 • edited Loading

Codecov Report

purna135 commented Mar 11, 2022

canyon289 commented Mar 11, 2022

purna135 commented Mar 11, 2022

ricardoV94 commented Mar 11, 2022 • edited Loading

purna135 commented Mar 11, 2022

ricardoV94 commented Mar 12, 2022

purna135 commented Mar 14, 2022 • edited Loading

ricardoV94 commented Mar 14, 2022

purna135 commented Mar 14, 2022 • edited Loading

ricardoV94 commented Mar 14, 2022

review-notebook-app bot commented Mar 15, 2022

purna135 commented Mar 15, 2022

twiecki commented Mar 15, 2022

purna135 commented Mar 15, 2022

twiecki commented Mar 16, 2022 via email

purna135 commented Mar 16, 2022

twiecki commented Mar 17, 2022

purna135 commented Mar 17, 2022

purna135 commented Mar 18, 2022

ricardoV94 commented Mar 18, 2022

ricardoV94 commented Mar 18, 2022

purna135 commented Mar 18, 2022

purna135 commented Mar 11, 2022 •

edited by ricardoV94

Loading

codecov bot commented Mar 11, 2022 •

edited

Loading

ricardoV94 commented Mar 11, 2022 •

edited

Loading

purna135 commented Mar 14, 2022 •

edited

Loading

purna135 commented Mar 14, 2022 •

edited

Loading