-
-
Notifications
You must be signed in to change notification settings - Fork 26k
DOC simpler block delimitation in sphinx-gallery examples #17068
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
DOC simpler block delimitation in sphinx-gallery examples #17068
Conversation
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.
IMHO, the current version makes the sections more obvious, especially in text mode. But why not
CI failure is unrelated due to |
Does anybody have a strong feeling about getting this in? |
Some IDEs such as vscode understand the formatting in this PR better, right? i.e. they understand each section as a separate cell. That maybe quite handy for people when they download the .py files I think. |
Yes, the following editors support See: sphinx-gallery/sphinx-gallery#518 (comment) Edit: updated links as some were out of date |
@adrinjalali but they don't understand |
@NicolasHug I don't think so |
Shouldn't the sections titles be already underlined in rst ? ################################################################
# Section title
# ------------- vs # %%
# Section title
# ------------- |
Some times
from: https://scikit-learn.org/dev/auto_examples/cluster/plot_mini_batch_kmeans.html |
@@ -46,7 +46,7 @@ | |||
logging.basicConfig(level=logging.INFO, format='%(asctime)s %(message)s') | |||
|
|||
|
|||
# ############################################################################# | |||
# %% |
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.
@rth , as noted by @thomasjpfan and @lucyleeow these ones should actually be interpreted as plain comments (this is on purpose according to #9061)
I can try to revert if you want
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.
They should be all in the first commit dd7cb01 (as I made a mistake in the regex the first time). If they are plain comments why do we need 80 chars of "#"? For instance in https://scikit-learn.org/dev/auto_examples/cluster/plot_mini_batch_kmeans.html I would just remove them, saying that we # Compute clustering with MiniBatchKMeans
doesn't actually need all those #
. We don't really comment that way usually, so I'm not sure why put them in examples. Also it's confusing because I really though they were there for sphinx.
But anyway it's a separate discussion happy to revert in this PR.
yes it's fine in genereal since there would be a title just below. But sometimes one would write Anyway no strong opinion. If IDEs cannot interpret the current |
@@ -164,6 +164,7 @@ | |||
# Learn a graphical structure from the correlations | |||
edge_model = covariance.GraphicalLassoCV() | |||
|
|||
# %% |
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 like there's a bunch of these that should not be added
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.
I added a few manually in 9a63b6e because I though they should have been cells in the first place. Can revert if you prefer.
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.
I would revert and have these adjustments be made separate PRs.
With this PR, we can keep the same formatting as master.
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.
I would revert these and have other PRs insert them.
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.
I would prefer not to adjust the formatting on master, since these are cosmetic choices.
@@ -66,7 +66,7 @@ | |||
|
|||
# ############################################################################# | |||
# Compute the likelihood on test data | |||
|
|||
# |
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.
@@ -164,6 +164,7 @@ | |||
# Learn a graphical structure from the correlations | |||
edge_model = covariance.GraphicalLassoCV() | |||
|
|||
# %% |
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.
I would revert and have these adjustments be made separate PRs.
With this PR, we can keep the same formatting as master.
# Download the data and make missing values sets | ||
################################################ | ||
# %% |
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.
This can be reverted. The original was correct.
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.
Revert this to make the above a header.
We would need to resolve the conflict. But the ability to execute the example directly from |
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.
A bunch of reverts to leave the rendered formatting alone.
(I am +1 on making this change.)
@@ -164,6 +164,7 @@ | |||
# Learn a graphical structure from the correlations | |||
edge_model = covariance.GraphicalLassoCV() | |||
|
|||
# %% |
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.
I would revert these and have other PRs insert them.
@@ -223,6 +224,7 @@ | |||
lc.set_linewidths(15 * values) | |||
ax.add_collection(lc) | |||
|
|||
# %% |
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.
Revert
# Use ``ColumnTransformer`` by selecting column by names | ||
############################################################################### | ||
# %% |
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.
# Using the prediction pipeline in a grid search | ||
############################################################################### | ||
# %% |
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.
Revert this to make the above a header.
# Download the data and make missing values sets | ||
################################################ | ||
# %% |
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.
Revert this to make the above a header.
# Create :class:`ConfusionMatrixDisplay` | ||
############################################################################## | ||
# %% |
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.
Revert this to make the above a header.
# Create :class:`RocCurveDisplay` | ||
############################################################################## | ||
# %% |
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.
Revert this to make the above a header.
# Create :class:`PrecisionRecallDisplay` | ||
############################################################################## | ||
# %% |
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.
Revert this to make the above a header.
# Combining the display objects into a single plot | ||
############################################################################## | ||
# %% |
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.
Revert this to make the above a header.
Should be good now. I have reverted all manual changes, and reverted section titles that were accidentally removed. |
The timeout on circleci is concerning. It is also happening on master as well. |
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.
Thank you @rth!
LGTM
…rn#17068) Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
…rn#17068) Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Closes #17063
On Linux,
(the syntax is slightly different for sed version installed on MacOS cf SO)
cc @thomasjpfan