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

DOC: add a tutorial about scipy.stats.qmc #13487

Merged
merged 9 commits into from
Mar 6, 2021
Merged

DOC: add a tutorial about scipy.stats.qmc #13487

merged 9 commits into from
Mar 6, 2021

Conversation

tupui
Copy link
Member

@tupui tupui commented Feb 1, 2021

This adds a general tutorial about QMC to explain how to use scipy.stats.qmc(#10844).

@rgommers rgommers added Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.stats labels Feb 1, 2021
V0lantis
V0lantis reviewed Feb 2, 2021
View reviewed changes
Copy link
Contributor

@V0lantis V0lantis left a comment

Choose a reason for hiding this comment

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

Very clear documentations ! I learnt something, thank you :)

@tupui
Copy link
Member Author

tupui commented Feb 2, 2021

Very clear documentations ! I learnt something, thank you :)

Thanks 😃 ! I am adding a few links to the doc and will unmark as draft.

@tupui tupui marked this pull request as ready for review February 2, 2021 12:17
@tupui
Copy link
Member Author

tupui commented Feb 2, 2021

@rgommers I have a first version. @ArtOwen proposed to help and add/improve the content. But I am not sure about the scope and the format here. So I wanted to check with you first with what I got now before asking him to work on something. Thanks for your guidance.

@rgommers
Copy link
Member

rgommers commented Feb 2, 2021

@rgommers I have a first version. @ArtOwen proposed to help and add/improve the content.

Awesome! Thanks for help @ArtOwen.

In case it's helpful, you can always access the latest version of the rendered html under the CI checks:

image

Making a QMC engine, i.e., subclassing QMCEngine

Subclassing tends to be fragile, and I hadn't realized you wanted to support that. It's fine, but it would be good to add some tests (I don't think there are any) so we don't inadvertently break end user subclasses.

So I wanted to check with you first with what I got now before asking him to work on something. Thanks for your guidance.

The format looks great. It's easy to follow, and starting with the general principles and then code examples is a good idea. It looks like it's in pretty good shape already. If you want to make it longer, we can break it out into a separate page.

@tupui
Copy link
Member Author

tupui commented Feb 2, 2021 •
edited

Subclassing tends to be fragile, and I hadn't realized you wanted to support that. It's fine, but it would be good to add some tests (I don't think there are any) so we don't inadvertently break end user subclasses.

I am actually not that sure we want to advertise this. As for the tests, right now there are just doctests you're right. TBH I am not sure how to properly test an ABC. Shall I just copy the doctest in the tests? So just do a basic subclass and test it with the TestQMCEngine class I have? I can remove this if you want, I don't have a strong opinion about this.

The format looks great. It's easy to follow, and starting with the general principles and then code examples is a good idea. It looks like it's in pretty good shape already. If you want to make it longer, we can break it out into a separate page.

Could almost be a complete module and not a submodule anymore 😉

@rgommers
Copy link
Member

rgommers commented Feb 2, 2021

So just do a basic subclass and test it with the TestQMCEngine class I have? I can remove this if you want, I don't have a strong opinion about this.

I think it's fine to keep. And yes, just copying into the tests - create a subclass, and call some methods on it. Here is how it's done for distribution subclasses: https://github.com/scipy/scipy/blob/master/scipy/stats/tests/test_distributions.py#L4680

@tupui tupui force-pushed the qmc_tutorial branch 2 times, most recently from 0aea2a3 to c837176 Compare February 3, 2021 15:19
@tupui tupui added this to the 1.7.0 milestone Feb 18, 2021
@rgommers
Copy link
Member

Thanks for the email summary @tupui. I thought it was still waiting for further changes. It looks in good shape already. Shall I review and merge now, and leave possible improvements to a next PR?

@tupui
Copy link
Member Author

tupui commented Feb 24, 2021

Thanks for the email summary @tupui. I thought it was still waiting for further changes. It looks in good shape already. Shall I review and merge now, and leave possible improvements to a next PR?

I am finished on my side. I think there is enough information and we can always add specifics if @ArtOwen has more input in the future 😃

chrisb83
chrisb83 reviewed Feb 25, 2021
View reviewed changes
Copy link
Member

@chrisb83 chrisb83 left a comment

Choose a reason for hiding this comment

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

I just had a quick look and left two comments.

doc/source/tutorial/stats.rst Outdated Show resolved Hide resolved
doc/source/tutorial/stats.rst Outdated Show resolved Hide resolved
rgommers
rgommers reviewed Feb 27, 2021
View reviewed changes
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

This looks good overall, thanks @tupui. I left some comments for parts I wasn't sure about, and pushed some textual fixes.

doc/source/tutorial/stats.rst Outdated Show resolved Hide resolved
doc/source/tutorial/stats.rst Outdated Show resolved Hide resolved
doc/source/tutorial/stats.rst Outdated Show resolved Hide resolved
doc/source/tutorial/stats.rst Show resolved Hide resolved
doc/source/tutorial/stats.rst Show resolved Hide resolved
doc/source/tutorial/stats.rst Outdated Show resolved Hide resolved
doc/source/tutorial/stats.rst Outdated Show resolved Hide resolved
doc/source/tutorial/stats.rst Show resolved Hide resolved
doc/source/tutorial/stats.rst Outdated Show resolved Hide resolved
doc/source/tutorial/stats/plots/qmc_plot_curse.py Outdated Show resolved Hide resolved
@tupui
Copy link
Member Author

tupui commented Feb 27, 2021

This looks good overall, thanks @tupui. I left some comments for parts I wasn't sure about, and pushed some textual fixes.

Thanks for the review!

chrisb83
chrisb83 reviewed Feb 27, 2021
View reviewed changes
doc/source/tutorial/stats.rst Outdated

f(\mathbf{x}) = \left( \sum_{j=1}^{5}x_j \right)^2,

which has a known mean value, :math:`\mu = 5/3+5(5-1)/4`. Using MC sampling, we
Copy link
Member

Choose a reason for hiding this comment

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

what kind of distribution do you assume for the x_i?

Copy link
Member Author

Choose a reason for hiding this comment

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

A uniform distribution. Usually when doing convergence analysis you only use uniform distribution as to not bias the results.

@tupui tupui mentioned this pull request Mar 5, 2021
Closed
rgommers
rgommers approved these changes Mar 6, 2021
View reviewed changes
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM now, merging. Thanks @tupui! And thanks to all the reviewers as well.

@rgommers rgommers merged commit 83205b1 into scipy:master Mar 6, 2021
@tupui
Copy link
Member Author

tupui commented Mar 6, 2021

Thank you @rgommers, and thank you everyone for reviewing 😃

@tupui tupui deleted the qmc_tutorial branch March 30, 2021 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkern rkern rkern left review comments

@chrisb83 chrisb83 chrisb83 left review comments

@V0lantis V0lantis V0lantis left review comments

@rgommers rgommers rgommers approved these changes

Assignees
No one assigned
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.stats
Projects
None yet
Milestone
1.7.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tupui @rgommers @rkern @chrisb83 @V0lantis