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

Updated plot method #308

Merged
merged 10 commits into from
Mar 13, 2023
Merged

Updated plot method #308

merged 10 commits into from
Mar 13, 2023

Conversation

Baronlegend27
Copy link
Collaborator

@Baronlegend27 Baronlegend27 commented Jan 5, 2023

Updated the plot method

@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Merging #308 (21c36ec) into master (8b4e886) will decrease coverage by 0.37%.
The diff coverage is 5.26%.

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
- Coverage   88.15%   87.79%   -0.37%     
==========================================
  Files          89       89              
  Lines        4281     4300      +19     
==========================================
+ Hits         3774     3775       +1     
- Misses        507      525      +18     
Impacted Files Coverage Δ
...py/discrete_distribution/_discrete_distribution.py 55.07% <5.26%> (-18.93%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Collaborator Author

@Baronlegend27 Baronlegend27 left a comment

Choose a reason for hiding this comment

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

I made the abstract method called plot. I am pretty sure everything is correct, however I wrote a lot of documentation for the method.

@Baronlegend27 Baronlegend27 changed the title Added the plot method Updated plot method Jan 5, 2023
Copy link
Collaborator

@alegresor alegresor left a comment

Choose a reason for hiding this comment

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

@Baronlegend27 This is a good start, the documentation for the method looks nice, I left some suggestions in this review. You still need to implement the method. This is not an abstract method but rather a method in the abstract class. Any subclass will inherit this plot method so you do not need to rewrite this method for every discrete distribution.

qmcpy/discrete_distribution/_discrete_distribution.py Outdated Show resolved Hide resolved
qmcpy/discrete_distribution/_discrete_distribution.py Outdated Show resolved Hide resolved
qmcpy/discrete_distribution/_discrete_distribution.py Outdated Show resolved Hide resolved
qmcpy/discrete_distribution/_discrete_distribution.py Outdated Show resolved Hide resolved
qmcpy/discrete_distribution/_discrete_distribution.py Outdated Show resolved Hide resolved
qmcpy/discrete_distribution/_discrete_distribution.py Outdated Show resolved Hide resolved
qmcpy/discrete_distribution/_discrete_distribution.py Outdated Show resolved Hide resolved
qmcpy/discrete_distribution/_discrete_distribution.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@alegresor alegresor left a comment

Choose a reason for hiding this comment

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

Looks better! Left a few more suggestions on how to improve the method.

You may also allow a user to pass in an ax argument which defaults to None. If an ax is passed in, you may just use that one to make the scatter plot on. If ax is None then you can create the fig,ax for the user.

Add some commands to pretty up the plot e.g. ones that set the aspect ratio, axis labels, axis ticks, axis limits

qmcpy/discrete_distribution/_discrete_distribution.py Outdated Show resolved Hide resolved
qmcpy/discrete_distribution/_discrete_distribution.py Outdated Show resolved Hide resolved
qmcpy/discrete_distribution/_discrete_distribution.py Outdated Show resolved Hide resolved
qmcpy/discrete_distribution/_discrete_distribution.py Outdated Show resolved Hide resolved
@alegresor alegresor merged commit ed2b3e1 into master Mar 13, 2023
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.

None yet

2 participants