Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removes BART #5566

Merged
merged 2 commits into from
Mar 10, 2022
Merged

Removes BART #5566

merged 2 commits into from
Mar 10, 2022

Conversation

aloctavodia
Copy link
Member

This removes BART from the main PyMC codebase in favor of moving it to pymc-experimental.

@ricardoV94
Copy link
Member

ricardoV94 commented Mar 8, 2022

We need to remove the test file from the pytest workflow:

pymc/tests/test_bart.py

--ignore=pymc/tests/test_bart.py

@ricardoV94
Copy link
Member

ricardoV94 commented Mar 8, 2022

What about the competence? Is there a level above Ideal that we can give to BART variables?

@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #5566 (4635bf3) into main (620b11d) will decrease coverage by 0.52%.
The diff coverage is 92.30%.

❗ Current head 4635bf3 differs from pull request most recent head 5f44242. Consider uploading reports for the commit 5f44242 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5566      +/-   ##
==========================================
- Coverage   88.13%   87.61%   -0.53%     
==========================================
  Files          81       76       -5     
  Lines       14238    13694     -544     
==========================================
- Hits        12549    11998     -551     
- Misses       1689     1696       +7     
Impacted Files Coverage Δ
pymc/__init__.py 100.00% <ø> (ø)
pymc/sampling.py 86.04% <90.00%> (-1.21%) ⬇️
pymc/step_methods/__init__.py 100.00% <100.00%> (ø)
pymc/step_methods/hmc/nuts.py 95.00% <100.00%> (-0.04%) ⬇️
pymc/step_methods/hmc/quadpotential.py 70.57% <0.00%> (-9.98%) ⬇️
pymc/parallel_sampling.py 87.70% <0.00%> (+0.99%) ⬆️
pymc/ode/ode.py 85.85% <0.00%> (+1.01%) ⬆️

@ricardoV94
Copy link
Member

Can you give more context what changes with this commit? 159e7bf

@ricardoV94 ricardoV94 added the BART label Mar 9, 2022
Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good,

Can you add a test for the step methods change, that shows you can add a step method externally?

Also can you split the bart changes from the step/methods assignment into 2 distinct commits (or PRs)?

@aloctavodia
Copy link
Member Author

What should I do to split the changes into 2 distinct commits (or PRs)?

@ricardoV94
Copy link
Member

What should I do to split the changes into 2 distinct commits (or PRs)?

git interactive rebase -> reorder + squash all the commits into two commits: 1) remove bart and 2) step method changes

@ricardoV94
Copy link
Member

@aloctavodia aloctavodia force-pushed the byebyebart branch 2 times, most recently from d70eda0 to 4635bf3 Compare March 9, 2022 15:43
Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

Can you change the commit messages to have a more descriptive title/messages? You don't need to include the titles of the squashed commits unless you want to. This can be done with interactive rebase + reword command

pymc/sampling.py Outdated Show resolved Hide resolved
@aloctavodia
Copy link
Member Author

I was confused with how to change the messages. Now I think I got it right.

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Award for best branch name! Just checking what is going on with read the docs failure

@ricardoV94 ricardoV94 merged commit 2dd4c8c into pymc-devs:main Mar 10, 2022
@ricardoV94
Copy link
Member

I merged it but just realized we need to mention it in the release notes @aloctavodia

@aloctavodia aloctavodia deleted the byebyebart branch March 11, 2022 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants