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.

Sign up for GitHub

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

Jump to bottom

Arrange distributions and sub-contents alphabetically #6653

Merged
merged 4 commits into from
Apr 13, 2023
Merged

Arrange distributions and sub-contents alphabetically #6653

merged 4 commits into from
Apr 13, 2023

Conversation

Dhruvanshu-Joshi
Copy link
Member

@Dhruvanshu-Joshi Dhruvanshu-Joshi commented Apr 6, 2023 •
edited by github-actions bot
Loading

What is this PR about?
Aimed to solve issue 6621.Updated the api docs for distributions and its sub-content to arrange the listed distributions in alphabetical order.

Checklist

Major / Breaking Changes

  • ...

New features

  • ...

Bugfixes

  • ...

Documentation

  • Did build sphinx locally and got the desired output. Distributions utilities is intentionally kept at the bottom in distributions.rtx.

Maintenance

  • ...

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

@ricardoV94 ricardoV94 changed the title Albhabeticaly arranging distributions ad its sub-contents Alphabeticaly arranging distributions ad its sub-contents Apr 7, 2023
@ricardoV94 ricardoV94 changed the title Alphabeticaly arranging distributions ad its sub-contents Arrase distributions ad sub-contents alphabetically Apr 7, 2023
@ricardoV94 ricardoV94 changed the title Arrase distributions ad sub-contents alphabetically Arrase distributions and sub-contents alphabetically Apr 7, 2023
@ricardoV94 ricardoV94 changed the title Arrase distributions and sub-contents alphabetically Arrange distributions and sub-contents alphabetically Apr 7, 2023
@ricardoV94
Copy link
Member

I am not sure we want alphabetically sorting at all levels... There's a question of what we prioritize as well

@Dhruvanshu-Joshi
Copy link
Member Author

Hey @ricardoV94 . Based on the discussion in issue 6621, I felt that sorting the distributions in alphabetical order is the next sensible step. How would you like me to change the order of the listed distributions?

@ricardoV94
Copy link
Member

I think the distributions is sensible but not necessarily the order of the categories (continuous vs discrete vs multivariate, etc). Others may disagree with me though.

@reshamas
Copy link
Contributor

reshamas commented Apr 7, 2023

I think the distributions is sensible but not necessarily the order of the categories (continuous vs discrete vs multivariate, etc). Others may disagree with me though.

  • I agree that the categories don't need to be in alphabetical order.
  • "Types of distributions" could be alphabetical. Continuous and discrete are the most widely referred (I assume). Alphabetical puts these at or near the top anyway: "Continuous" and "Categorical"
  • Within "Continuous", if those distributions were in alphabetical order, it would be much easier to find, especially since there are 30+ types of continuous distributions:
    • Beta
    • Cauchy
    • etc.
  • Same for "Discrete".

@Dhruvanshu-Joshi
Copy link
Member Author

Thank You @ricardoV94 @reshamas . Just so that I get this right this time, I'll summarize the changes to be made that I understand. Firstly, keep categories( i.e. continuous vs discrete vs multivariate, etc) in the same order as is currently displayed on the website. Then, alphabetically arrange the types of distributions under each category( eg: continuous, discrete, etc).

@codecov
Copy link

codecov bot commented Apr 7, 2023 •
edited
Loading

Codecov Report

Merging #6653 (3234c29) into main (5d68bf3) will not change coverage.
The diff coverage is n/a.

❗ Current head 3234c29 differs from pull request most recent head 6f404f2. Consider uploading reports for the commit 6f404f2 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6653   +/-   ##
=======================================
  Coverage   91.97%   91.97%           
=======================================
  Files          94       94           
  Lines       15942    15942           
=======================================
  Hits        14663    14663           
  Misses       1279     1279

@ricardoV94 ricardoV94 added the docs label Apr 9, 2023
larryshamalama
larryshamalama approved these changes Apr 9, 2023
View reviewed changes
Copy link
Member

@larryshamalama larryshamalama 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, thanks @Dhruvanshu-Joshi 🙂 I also notice that you added the distributions that were forgotten in the documentation, which is much appreciated!

reshamas
reshamas reviewed Apr 9, 2023
View reviewed changes
docs/source/api/distributions/continuous.rst Outdated
Comment on lines 25 to 27
Laplace
Logistic
LogNormal
LogitNormal
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Laplace
Logistic
LogNormal
LogitNormal
Laplace
Logistic
LogitNormal
LogNormal

@Dhruvanshu-Joshi
Copy link
Member Author

Looks good, thanks @Dhruvanshu-Joshi 🙂 I also notice that you added the distributions that were forgotten in the documentation, which is much appreciated!

Thank You @larryshamalama ☺️

@Dhruvanshu-Joshi
Copy link
Member Author

Hey @reshamas , I have included the suggestion in the latest commit. Hope it solves the issue.

reshamas
reshamas reviewed Apr 9, 2023
View reviewed changes
docs/source/api/distributions/continuous.rst Outdated
Comment on lines 36 to 37
TruncatedNormal
Triangular
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TruncatedNormal
Triangular
Triangular
TruncatedNormal

reshamas
reshamas reviewed Apr 9, 2023
View reviewed changes
docs/source/api/distributions/mixture.rst Outdated
Comment on lines 9 to 10
Mixture
NormalMixture
Mixture
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't "Mixture" come before "NormalMixture"?

reshamas
reshamas reviewed Apr 9, 2023
View reviewed changes
docs/source/api/distributions/utilities.rst Outdated
Comment on lines 11 to 12
Distribution
Discrete
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Distribution
Discrete
Discrete
Distribution

@reshamas
Copy link
Contributor

Thank you @Dhruvanshu-Joshi.
I have no more comments. Looks great.

@reshamas
Copy link
Contributor

@ricardoV94
This is a doc fix, so it seems not all the checks are running.

cc: @OriolAbril

@ricardoV94
Copy link
Member

We didn't change the required yet to not force old PRs to update.

@ricardoV94 ricardoV94 merged commit aae97a2 into pymc-devs:main Apr 13, 2023
@ricardoV94 ricardoV94 mentioned this pull request Apr 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reshamas reshamas reshamas left review comments

@larryshamalama larryshamalama larryshamalama approved these changes

@ricardoV94 ricardoV94 ricardoV94 approved these changes

Assignees
No one assigned
Labels
docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Dhruvanshu-Joshi @ricardoV94 @reshamas @larryshamalama