Renamed SMC files #6174

IMvision12 · 2022-10-02T07:46:17Z

What is this PR about?

Renamed SMC Files
Closes : #6171

Major / Breaking Changes

Renamed SMC files

ricardoV94

Thanks for opening the PR. We should keep the internal function names unchanged. Change only file names.

RELEASE-NOTES.md

pymc/distributions/simulator.py

pymc/smc/__init__.py

pymc/smc/sampling.py

ricardoV94 · 2022-10-02T12:41:22Z

@IMvision12 Why did you close #6173 and open this PR?

IMvision12 · 2022-10-02T13:04:35Z

No particular reason; did I do something wrong?

Armavica · 2022-10-02T14:04:52Z

No particular reason; did I do something wrong?

If you are not happy with a PR that you submitted, or if the tests don't pass, etc., there is no need to create a new PR: the easiest is to push again to the same branch, and the PR will be updated. If you remove or modify some of the commits that are already pushed, you can git push --force to replace them. A PR is a request to merge some changes but while it has not been accepted you can always change what you want to merge.

IMvision12 · 2022-10-02T14:08:52Z

Thanks for Information

pymc/tests/distributions/test_simulator.py

pymc/smc/sampling.py

pymc/tests/smc/test_smc.py

Armavica

Following up on @ricardoV94's review, I pointed out the places that should not be modified.

pymc/distributions/simulator.py

pymc/smc/sampling.py

pymc/tests/distributions/test_simulator.py

pymc/tests/smc/test_smc.py

docs/source/api/smc.rst

IMvision12 · 2022-10-04T05:19:24Z

@Armavica Thanks for feedback, I have updated the files

Armavica

There are still two places in test_smc.py that have been modified but shouldn't be. There are also a few places in this file that have not been modified but should be, you will see them if you run the tests with pytest pymc/tests/smc. After that, I think that it should be good.

pymc/tests/smc/test_smc.py

IMvision12 · 2022-10-05T08:27:07Z

@Armavica I modified the file, but the tests are still failing.

codecov · 2022-10-05T08:55:00Z

Codecov Report

Merging #6174 (d1a4f5c) into main (570e6e8) will decrease coverage by 8.12%.
The diff coverage is 66.66%.

❗ Current head d1a4f5c differs from pull request most recent head 451c439. Consider uploading reports for the commit 451c439 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6174      +/-   ##
==========================================
- Coverage   93.77%   85.65%   -8.13%     
==========================================
  Files         101       99       -2     
  Lines       22232    20962    -1270     
==========================================
- Hits        20849    17955    -2894     
- Misses       1383     3007    +1624

Impacted Files	Coverage Δ
pymc/smc/kernels.py	`76.02% <ø> (ø)`
pymc/tests/smc/test_smc.py	`0.00% <0.00%> (-100.00%)`	⬇️
pymc/smc/sampling.py	`77.85% <100.00%> (ø)`
pymc/tests/distributions/test_simulator.py	`99.50% <100.00%> (ø)`
pymc/tests/tuning/test_scaling.py	`0.00% <0.00%> (-100.00%)`	⬇️
pymc/tests/tuning/test_starting.py	`0.00% <0.00%> (-100.00%)`	⬇️
pymc/tests/step_methods/test_slicer.py	`0.00% <0.00%> (-100.00%)`	⬇️
pymc/tests/step_methods/hmc/test_nuts.py	`0.00% <0.00%> (-100.00%)`	⬇️
pymc/tests/distributions/test_transform.py	`0.00% <0.00%> (-100.00%)`	⬇️
pymc/tests/step_methods/test_metropolis.py	`0.00% <0.00%> (-100.00%)`	⬇️
... and 50 more

Armavica · 2022-10-05T13:40:50Z

Some of these tests fail because there are still a few places in test_smc.py that should be updated as a consequence of the file name change. You can see the output of the relevant tests on your computer if you run pytest pymc/tests/smc, or also here: https://github.com/pymc-devs/pymc/actions/runs/3188204616/jobs/5201000626 Please let me know if you need help fixing these issues.

Some others fail because now pymc.sampling is colliding with pymc.smc.sampling, because of

pymc/pymc/__init__.py

Line 71 in a135f53

from pymc.smc import *

@ricardoV94 what should we do here?

ricardoV94 · 2022-10-28T08:11:30Z

Some of these tests fail because there are still a few places in test_smc.py that should be updated as a consequence of the file name change. You can see the output of the relevant tests on your computer if you run pytest pymc/tests/smc, or also here: https://github.com/pymc-devs/pymc/actions/runs/3188204616/jobs/5201000626 Please let me know if you need help fixing these issues.

Some others fail because now pymc.sampling is colliding with pymc.smc.sampling, because of

pymc/pymc/__init__.py

Line 71 in a135f53

from pymc.smc import *

@ricardoV94 what should we do here?

I pushed some changes that specify __all__ so that not everything is imported at the root level

ricardoV94 · 2022-10-28T08:15:23Z

@IMvision12 I squashed the commits and force-pushed do solve the conflicts. Be careful to get the upstream changes before you try to do further changes

michaelosthege

One small comment, otherwise this appears to be ready to merge?

michaelosthege · 2022-10-28T19:29:23Z

pymc/smc/__init__.py

+from pymc.smc.kernels import IMH, MH
+from pymc.smc.sampling import sample_smc
+
+__all__ = ("sample_smc",)


@IMvision12 you didn't include IMH and MH in __all__?

They weren't available at the root level before, just via pymc.smc. I think it was fine like that

Armavica · 2022-10-28T22:57:56Z

pymc/smc/__init__.py

+from pymc.smc.kernels import IMH, MH
+from pymc.smc.sampling import sample_smc
+
+__all__ = ("sample_smc",)


nit: Isn't __all__ usually a list? In PyMC it is a list in 42 places and a tuple in only 2.

I'm team tuples

ricardoV94 requested changes Oct 2, 2022

View reviewed changes

RELEASE-NOTES.md Outdated Show resolved Hide resolved

RELEASE-NOTES.md Outdated Show resolved Hide resolved

pymc/distributions/simulator.py Outdated Show resolved Hide resolved

pymc/smc/__init__.py Outdated Show resolved Hide resolved

pymc/smc/sampling.py Outdated Show resolved Hide resolved

IMvision12 requested a review from ricardoV94 October 2, 2022 10:14

Armavica requested changes Oct 2, 2022

View reviewed changes

pymc/tests/distributions/test_simulator.py Outdated Show resolved Hide resolved

pymc/smc/sampling.py Outdated Show resolved Hide resolved

pymc/tests/smc/test_smc.py Outdated Show resolved Hide resolved

IMvision12 requested review from Armavica and removed request for ricardoV94 October 3, 2022 04:53

Armavica requested changes Oct 3, 2022

View reviewed changes

IMvision12 requested a review from Armavica October 4, 2022 05:19

Armavica requested changes Oct 4, 2022

View reviewed changes

pymc/tests/smc/test_smc.py Outdated Show resolved Hide resolved

pymc/tests/smc/test_smc.py Outdated Show resolved Hide resolved

Armavica added no releasenotes Skipped in automatic release notes generation maintenance SMC Sequential Monte Carlo labels Oct 5, 2022

ricardoV94 removed the no releasenotes Skipped in automatic release notes generation label Oct 28, 2022

Rename SMC files

451c439

ricardoV94 requested review from Armavica and ricardoV94 October 28, 2022 08:16

ricardoV94 added this to the v4.3.0 milestone Oct 28, 2022

ricardoV94 added the major Include in major changes release notes section label Oct 28, 2022

michaelosthege approved these changes Oct 28, 2022

View reviewed changes

Armavica approved these changes Oct 28, 2022

View reviewed changes

ricardoV94 approved these changes Oct 29, 2022

View reviewed changes

ricardoV94 merged commit bcffce2 into pymc-devs:main Oct 29, 2022

reshamas mentioned this pull request Nov 8, 2022

Update docstrings for sample_smc and smc.py #6114

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renamed SMC files #6174

Renamed SMC files #6174

IMvision12 commented Oct 2, 2022 •

edited by ricardoV94

Loading

ricardoV94 left a comment •

edited

Loading

ricardoV94 commented Oct 2, 2022 •

edited

Loading

IMvision12 commented Oct 2, 2022

Armavica commented Oct 2, 2022

IMvision12 commented Oct 2, 2022 •

edited

Loading

Armavica left a comment

IMvision12 commented Oct 4, 2022

Armavica left a comment

IMvision12 commented Oct 5, 2022

codecov bot commented Oct 5, 2022 •

edited

Loading

Armavica commented Oct 5, 2022

ricardoV94 commented Oct 28, 2022

ricardoV94 commented Oct 28, 2022

michaelosthege left a comment

michaelosthege Oct 28, 2022

ricardoV94 Oct 28, 2022

Armavica Oct 28, 2022

ricardoV94 Oct 29, 2022

Renamed SMC files #6174

Renamed SMC files #6174

Conversation

IMvision12 commented Oct 2, 2022 • edited by ricardoV94 Loading

Major / Breaking Changes

ricardoV94 left a comment • edited Loading

Choose a reason for hiding this comment

ricardoV94 commented Oct 2, 2022 • edited Loading

IMvision12 commented Oct 2, 2022

Armavica commented Oct 2, 2022

IMvision12 commented Oct 2, 2022 • edited Loading

Armavica left a comment

Choose a reason for hiding this comment

IMvision12 commented Oct 4, 2022

Armavica left a comment

Choose a reason for hiding this comment

IMvision12 commented Oct 5, 2022

codecov bot commented Oct 5, 2022 • edited Loading

Codecov Report

Armavica commented Oct 5, 2022

ricardoV94 commented Oct 28, 2022

ricardoV94 commented Oct 28, 2022

michaelosthege left a comment

Choose a reason for hiding this comment

michaelosthege Oct 28, 2022

Choose a reason for hiding this comment

ricardoV94 Oct 28, 2022

Choose a reason for hiding this comment

Armavica Oct 28, 2022

Choose a reason for hiding this comment

ricardoV94 Oct 29, 2022

Choose a reason for hiding this comment

IMvision12 commented Oct 2, 2022 •

edited by ricardoV94

Loading

ricardoV94 left a comment •

edited

Loading

ricardoV94 commented Oct 2, 2022 •

edited

Loading

IMvision12 commented Oct 2, 2022 •

edited

Loading

codecov bot commented Oct 5, 2022 •

edited

Loading