-
Notifications
You must be signed in to change notification settings - Fork 22
Port examples to sphinx_gallery #170
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
Conversation
58332eb
to
b3c2039
Compare
Somebody can help me with the linter? I ran black locally but still got an error in the ci... EDIT: Should be fixed. My local version of black was not up to date. |
d836803
to
5a5ad37
Compare
I just saw that we can even use jupyterlite allowing running the examples in the browser. Should we try this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks nice, but I caught a few conversion artifacts here and there. I have two other issues that should be ameliorated before merge --
- Ordering of the gallery -- we previously had the examples in a specific order, can we preserve this for the gallery?
- Sidebar contains all sub-headers, and should not.
I've built the docs at https://scikit-matter.readthedocs.io/en/sphinx_gallery/index.html if you'd like to see.
|
||
# %% | ||
# | ||
# Scale and Center the Features and Targets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've lost the relative importance of each header, can you fix it so that they are stacked like this?
- Feature Selection on the WHO Dataset
- Load the Dataset
- Scale and Center the Features and Targets
- Provide an estimated target for the feature selector
- Compute the Selections for Each Selector Type
- PCov-CUR
- PCov-FPS
- CUR
- FPS
- (For Comparison) Recursive Feature Addition
- Plot our Results
- Load the Dataset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be done in the latest version.
|
||
# %% | ||
# | ||
# (For Comparison) Recurisive Feature Addition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# (For Comparison) Recurisive Feature Addition | |
# (For Comparison) Recursive Feature Addition |
|
||
plt.legend(bbox_to_anchor=(1, 1), loc="upper left") | ||
|
||
plt.show() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh this looks nice 🤣.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets see how this looks in the new commit.
) | ||
ax_wo_orth.set_xlabel("scaling in z direction") | ||
ax_wo_orth.legend(loc="upper right", bbox_to_anchor=(0.7, -0.2)) | ||
plt.show() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets see how this looks in the new commit.
examples/PlotLFRE.py
Outdated
|
||
# %% | ||
# | ||
# load features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as last example
examples/PlotPointwiseGFRE.py
Outdated
|
||
# %% | ||
# | ||
# load features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as last two
examples/PlotPointwiseGFRE.py
Outdated
# \exp(-\gamma \|\mathbf{x}-\mathbf{x}'\|^2),\quad \gamma\in\mathbb{R}_+ | ||
# | ||
# The projected RKHS features are computed using the eigendecomposition of the | ||
# positive-definite kernel matrix :math:`K`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prime mark is misplaced
Feel free to put in an issue, I think that'd be great! |
Thanks for the feedback. I will see what I can do. Manual ordering always works. |
Thanks but you don't have to enable this manually the next time. In #161 we enabled that RTD builds the docs for each PR. For this one it is here: https://scikit-matter--170.org.readthedocs.build/en/170/. I will add a GH action in #172 for a direct link. |
6e6b9a8
to
9cf0239
Compare
8bb6c36
to
81a67bd
Compare
Just went through the examples and made some typo/formatting fixes. Rest looks good. |
f1aa078
to
6834940
Compare
Thanks @victorprincipe. All of them are super useful changes. I fixed another tiny formatting thing. |
6834940
to
26a68fc
Compare
26a68fc
to
ef1d0b7
Compare
I don't see the nice gallery on the auto-built readthedocs page at https://scikit-matter--170.org.readthedocs.build/en/170/tutorials.html# |
I had to remove the gallery view in order to keep the same structure as it was before. Sorry, if we want to have the gallery back I can look later how we can order it. But I think it will be not super easy to maintain the section titles. |
Bummer. The gallery was really pretty. |
Yes, I was also a fan of it. |
Is there no way to preserve order with the gallery?
…On Tue, Apr 4, 2023, 16:25 Philip Loche ***@***.***> wrote:
Yes, I was also a fan of it.
—
Reply to this email directly, view it on GitHub
<#170 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALKVP3TXKCKCVRUIC72N6J3W7SGUJANCNFSM6AAAAAAWHNV6P4>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
The only way I see is that we do subfolders for each current section and then we use sub-gallery-order. |
34d4553
to
044248e
Compare
044248e
to
26d2332
Compare
I like it a lot - it's also better to have separate pages, and I was finding the list of sub-headings of the notebooks rather confusing. IMO this is good to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Please squash before merging.
Thanks for your feedback! I can help you with rebasing your PR @ceriottm |
Fixes #167
As already stated in the issue this will allow easier maintance and gives these nice gallery view:
Also i fixed some warnings when generating the docs.