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

Notebook to sphinx-gallery script #4854

Merged
merged 6 commits into from Oct 27, 2023

Conversation

MRegeard
Copy link
Member

This PR is related to issue #4441.

It adds a script in the dev folder that allows to convert jupyter notebook in the sphinx-gallery python script format. This is especially useful when creating a tutorial. First create it as a jupyter notebook, and them convert it into a python script when the tutorial is ready to be reviewed.

This PR also adds a small section in the dev how-to documentation that explain how to use this script.

Since this script uses pypandoc, I also added pypandoc to the pip section of the environment file. This is not a big deal because we already have pandoc in the environment file and pypandoc is just a python wrapper for pandoc.
I put it in the pip section because it is not available for os-arm in the conda-forge repository.

Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #4854 (ec2f774) into main (d811f92) will decrease coverage by 0.02%.
Report is 24 commits behind head on main.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##             main    #4854      +/-   ##
==========================================
- Coverage   75.63%   75.62%   -0.02%     
==========================================
  Files         226      226              
  Lines       33029    33069      +40     
==========================================
+ Hits        24983    25007      +24     
- Misses       8046     8062      +16     
Files Coverage Δ
gammapy/modeling/models/core.py 84.45% <33.33%> (-0.30%) ⬇️
gammapy/modeling/models/tests/test_core.py 76.60% <38.46%> (-3.14%) ⬇️

... and 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@bkhelifi bkhelifi left a comment

Choose a reason for hiding this comment

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

Thanks @MRegeard...
A naive question: have you try to take a notebook build by our doc (e.g. docs/tutorials/api/makers.ipynb) and compare the output of this code to the initial python code?

Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard
Copy link
Member Author

@bkhelifi, Yes I checked that !

The only thing that is different is that it put =====under the "title" of the notebook while it is currently ----- in our notebooks. But I checked and it does not affect the rendering on the documentation.

The other thing that it was doing was adding a

"`"

at the beginning and at the end when we made reference to the code using ~gammapy.*, even though one was already there. That's why I added a replace("``", "`") after generating the string.

Tell me if anyone has a strong opinion on the `====, I will find a way !

AtreyeeS
AtreyeeS previously approved these changes Oct 25, 2023
Copy link
Member

@AtreyeeS AtreyeeS 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 for this much needed addition.

Maybe you can build the docs locally on your machine once to ensure that the rendered notebooks have no issues with the ====

@MRegeard
Copy link
Member Author

@AtreyeeS, That's what I have done ! So it does not change anything, at least that I could notice.

But I just notice that we are using indistinctly ==== and ----- for the "title" of the tutorial. So we might want to uniform that anyway.

Copy link
Member

@Astro-Kirsty Astro-Kirsty left a comment

Choose a reason for hiding this comment

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

Thanks @MRegeard.
Just made some minor comments!

dev/ipynb_to_gallery.py Outdated Show resolved Hide resolved
dev/ipynb_to_gallery.py Outdated Show resolved Hide resolved
dev/ipynb_to_gallery.py Outdated Show resolved Hide resolved
dev/ipynb_to_gallery.py Outdated Show resolved Hide resolved
registerrier
registerrier previously approved these changes Oct 26, 2023
Copy link
Contributor

@registerrier registerrier left a comment

Choose a reason for hiding this comment

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

Thanks @MRegeard . This is very useful!

I have left a few minor comments inline.

dev/ipynb_to_gallery.py Show resolved Hide resolved
dev/ipynb_to_gallery.py Outdated Show resolved Hide resolved
Comment on lines 50 to 56
if output_file_name is None:
output_file_name = input_file_name
open(output_file_name.replace(".ipynb", ".py"), "w").write(python_file)
else:
if not output_file_name.endswith(".py"):
output_file_name = output_file_name + ".py"
open(output_file_name, "w").write(python_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if output_file_name is None:
output_file_name = input_file_name
open(output_file_name.replace(".ipynb", ".py"), "w").write(python_file)
else:
if not output_file_name.endswith(".py"):
output_file_name = output_file_name + ".py"
open(output_file_name, "w").write(python_file)
if output_file_name is None:
output_file_name = input_file_name.replace(".ipynb", ".py")
if not output_file_name.endswith(".py"):
output_file_name = output_file_name + ".py"
open(output_file_name, "w").write(python_file)

less indentation is usually better

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that you could also use the gammapy.utils.script.make_path function which allows you to use pathlib.Path objects with e.g. Path.write_text(python_file) or Path.with_suffix(".py")

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it's true that I could have used that.
Now that it is written like this, do you see any reason why we should switch to Path?

dev/ipynb_to_gallery.py Outdated Show resolved Hide resolved
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard MRegeard dismissed stale reviews from registerrier and AtreyeeS via ec2f774 October 26, 2023 13:58
@MRegeard
Copy link
Member Author

@registerrier, @Astro-Kirsty, suggestion implemented !

Copy link
Member

@bkhelifi bkhelifi left a comment

Choose a reason for hiding this comment

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

Thanks @MRegeard.. From my side, nothing more to add

@registerrier registerrier merged commit b34bbe3 into gammapy:main Oct 27, 2023
12 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding notebook to script documentation in dev doc
5 participants