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

Move CustomDist logic to dedicated module and docs section #7363

Merged

Conversation

markgreene74
Copy link
Contributor

@markgreene74 markgreene74 commented Jun 15, 2024

@Hemant19870601 and I worked on this PR during the PyMC Hackathon at PyData London 2024.

Description

With this PR CustomDist is moved to a separate file (custom_distribution.py) along with related classes (CustomDistRV, CustomDistRV, CustomSymbolicDistRV, _CustomSymbolicDist) and helper functions (default_not_implemented, default_support_point).

The imports in distribution.py and __init__.py have been updated to reflect the changes.

DensityDist has been moved to the new file as well.

Note that in distribution.py the import of CustomSymbolicDistRV has been added close to where the class is used to avoid circular import. A noqa has been added on the same line to avoid a ruff error for the misplaced import (E402).

Similarly, the CustomDist tests have been moved to their own file (test_custom_distribution.py) and are passing.

Test results (before/after):

file before (main branch) after
test_distribution.py 71 pass/14 ignored/85 total 33 pass/0 ignored/33 total
test_custom_distribution.py N/A 38 pass/14 ignored/52 total
71 pass/14 ignored/85 total 71 pass/14 ignored/85 total

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify): Refactor

📚 Documentation preview 📚: https://pymc--7363.org.readthedocs.build/en/7363/

Copy link

welcome bot commented Jun 15, 2024

Thank You Banner]
💖 Thanks for opening this pull request! 💖 The PyMC community really appreciates your time and effort to contribute to the project. Please make sure you have read our Contributing Guidelines and filled in our pull request template to the best of your ability.

tests/distributions/test_custom_distribution.py Outdated Show resolved Hide resolved
@markgreene74
Copy link
Contributor Author

Hi @ricardoV94 thank you for all the comments :) I think this PR should be ready for review now, happy for me to change the status?

@twiecki twiecki marked this pull request as ready for review June 23, 2024 20:34
@ricardoV94 ricardoV94 force-pushed the issue-7247-move-custom-dist-logic branch from e365f4c to 2333e5e Compare June 24, 2024 08:37
@ricardoV94
Copy link
Member

I rebased the PR on top of main to solve the conflicts with #7370

@ricardoV94 ricardoV94 force-pushed the issue-7247-move-custom-dist-logic branch from 2333e5e to 5935250 Compare June 24, 2024 08:46
@ricardoV94 ricardoV94 changed the title Move CustomDist logic to a dedicated file Move CustomDist logic to dedicated module and docs section Jun 24, 2024
@ricardoV94 ricardoV94 added the major Include in major changes release notes section label Jun 24, 2024
@ricardoV94 ricardoV94 force-pushed the issue-7247-move-custom-dist-logic branch from bcfc020 to dc8b2d4 Compare June 24, 2024 08:54
ricardoV94 and others added 5 commits June 24, 2024 11:06
Co-authored-by: Giuseppe Cunsolo <markgreene74@users.noreply.github.com>
Co-authored-by: markgreene74@users.noreply.github.com
Co-authored-by: "Hemant19870601" <hemant.thakare01@gmail.com>
Co-authored-by: Giuseppe Cunsolo <markgreene74@users.noreply.github.com>
Co-authored-by: markgreene74@users.noreply.github.com
Co-authored-by: "Hemant19870601" <hemant.thakare01@gmail.com>
@ricardoV94 ricardoV94 force-pushed the issue-7247-move-custom-dist-logic branch from dc8b2d4 to 9da36c3 Compare June 24, 2024 09:26
@ricardoV94 ricardoV94 requested a review from twiecki June 24, 2024 09:28
Copy link

codecov bot commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 94.59459% with 14 lines in your changes missing coverage. Please review.

Project coverage is 92.19%. Comparing base (e71d1cb) to head (9da36c3).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7363   +/-   ##
=======================================
  Coverage   92.18%   92.19%           
=======================================
  Files         102      103    +1     
  Lines       17199    17214   +15     
=======================================
+ Hits        15855    15870   +15     
  Misses       1344     1344           
Files Coverage Δ
pymc/distributions/__init__.py 100.00% <100.00%> (ø)
pymc/distributions/distribution.py 92.09% <100.00%> (-0.77%) ⬇️
pymc/distributions/truncated.py 99.45% <100.00%> (+<0.01%) ⬆️
pymc/gp/util.py 97.56% <100.00%> (-0.06%) ⬇️
pymc/model/core.py 91.75% <100.00%> (ø)
pymc/distributions/custom.py 94.46% <94.46%> (ø)

@ricardoV94 ricardoV94 merged commit 7af0a87 into pymc-devs:main Jun 24, 2024
21 of 22 checks passed
Copy link

welcome bot commented Jun 24, 2024

Congratulations Banner]
Congrats on merging your first pull request! 🎉 We here at PyMC are proud of you! 💖 Thank you so much for your contribution 🎁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs maintenance major Include in major changes release notes section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CustomDist as second level header below Distributions in the API Move CustomDist logic to its own file
3 participants