Skip to content

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

Merged
merged 11 commits into from
Jun 9, 2020

Conversation

rth
Copy link
Member

@rth rth commented Apr 28, 2020

Closes #17063

On Linux,

find examples/ -name "*.py" -exec sed -i 's/#\s*#\{5,100\}/# %%/g' {} +

(the syntax is slightly different for sed version installed on MacOS cf SO)

cc @thomasjpfan

@adrinjalali
Copy link
Member

Nice!

Copy link
Member

@NicolasHug NicolasHug left a 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

@rth
Copy link
Member Author

rth commented Apr 28, 2020

CI failure is unrelated due to fetch_california_housing network error.

@thomasjpfan
Copy link
Member

Some of these do change the formatting. For example: this pr and master.

@NicolasHug
Copy link
Member

NicolasHug commented Apr 29, 2020

Does anybody have a strong feeling about getting this in?

@adrinjalali
Copy link
Member

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.

@lucyleeow
Copy link
Member

lucyleeow commented Apr 29, 2020

Yes, the following editors support # %%/#%%:

See: sphinx-gallery/sphinx-gallery#518 (comment)

Edit: updated links as some were out of date

@NicolasHug
Copy link
Member

@adrinjalali but they don't understand ###########... ?

@lucyleeow
Copy link
Member

@NicolasHug I don't think so

@rth
Copy link
Member Author

rth commented Apr 29, 2020

IMHO, the current version makes the sections more obvious, especially in text mode

Shouldn't the sections titles be already underlined in rst ?

################################################################
# Section title
# -------------

vs

# %%
# Section title
# -------------

@lucyleeow
Copy link
Member

Some times ###'s are used to separate parts of code without a new section being defined, e.g.,:

# #############################################################################
# Generate sample data
np.random.seed(0)

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')


# #############################################################################
# %%
Copy link
Member

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

Copy link
Member Author

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.

@NicolasHug
Copy link
Member

Shouldn't the sections titles be already underlined in rst ?

yes it's fine in genereal since there would be a title just below. But sometimes one would write ######## with no direct title, for example the highlights which render like this

Anyway no strong opinion. If IDEs cannot interpret the current ####### then I guess that's a good reason to switch.

@@ -164,6 +164,7 @@
# Learn a graphical structure from the correlations
edge_model = covariance.GraphicalLassoCV()

# %%
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

@thomasjpfan thomasjpfan left a 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

#
Copy link
Member

Choose a reason for hiding this comment

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

Some of these single # does not change the formatting. In this example, PR and master

This is most likely because the ##### is not connected.

@@ -164,6 +164,7 @@
# Learn a graphical structure from the correlations
edge_model = covariance.GraphicalLassoCV()

# %%
Copy link
Member

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
################################################
# %%
Copy link
Member

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.

Copy link
Member

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.

@glemaitre
Copy link
Member

We would need to resolve the conflict. But the ability to execute the example directly from vs code is a good alternative to starting a jupyter notebook. I think this is worth merging those.

Copy link
Member

@thomasjpfan thomasjpfan left a 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()

# %%
Copy link
Member

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)

# %%
Copy link
Member

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
###############################################################################
# %%
Copy link
Member

Choose a reason for hiding this comment

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

Revert this.

The second ########################## was used for a header:

This PR:

Screen Shot 2020-05-26 at 6 13 07 PM

Master:

Screen Shot 2020-05-26 at 6 13 23 PM

# Using the prediction pipeline in a grid search
###############################################################################
# %%
Copy link
Member

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
################################################
# %%
Copy link
Member

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`
##############################################################################
# %%
Copy link
Member

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`
##############################################################################
# %%
Copy link
Member

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`
##############################################################################
# %%
Copy link
Member

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
##############################################################################
# %%
Copy link
Member

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.

@rth
Copy link
Member Author

rth commented Jun 3, 2020

Should be good now. I have reverted all manual changes, and reverted section titles that were accidentally removed.

@thomasjpfan
Copy link
Member

The timeout on circleci is concerning. It is also happening on master as well.

Copy link
Member

@thomasjpfan thomasjpfan left a 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

@thomasjpfan thomasjpfan merged commit 94f2e9c into scikit-learn:master Jun 9, 2020
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
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.

Replace "######" separators by "# %%" in gallery
6 participants