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

Adjust getting-started page #4983

Merged
merged 9 commits into from
Mar 11, 2024

Conversation

Astro-Kirsty
Copy link
Member

@Astro-Kirsty Astro-Kirsty commented Dec 13, 2023

Description
This PR is to resolve #4838. It adjusts the Getting started page to be far more simple and realistic to how we would typically install gammapy.

Then the other pages (Installation, Virtual Environments etc) are adjusted accordingly.

https://astro-kirsty.github.io/gammapy-docs-preview/

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.23%. Comparing base (a711d11) to head (b43b007).
Report is 345 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4983      +/-   ##
==========================================
- Coverage   75.41%   75.23%   -0.18%     
==========================================
  Files         230      234       +4     
  Lines       34122    34845     +723     
==========================================
+ Hits        25733    26216     +483     
- Misses       8389     8629     +240     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MRegeard
Copy link
Member

Since we have a few dependencies in the explicit pip channel of the environment file, I moved sherpa in this section so that we don't need the note on sherpa with Apple Silicon in the quickstart page of the documentation.
Now sherpa installation should work on all Linux and Apple OS.

cgalelli
cgalelli previously approved these changes Dec 14, 2023
Copy link
Contributor

@cgalelli cgalelli left a comment

Choose a reason for hiding this comment

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

Thanks @Astro-Kirsty for this useful set of adjustments!

@adonath
Copy link
Member

adonath commented Dec 14, 2023

Thanks @Astro-Kirsty! Can you locally build the documentation and push a preview to your GitHub? I did this in the past e.g. here: https://github.com/adonath/gammapy-docs-preview

It should work like:

make docs-sphinx
cp -r docs/_build/html path-to/gammapy-docs-preview

Then we can access the docs online at Astro-Kirsty.github.io/gammapy-docs-preview.

@Astro-Kirsty
Copy link
Member Author

Astro-Kirsty commented Dec 14, 2023

I've made the repo, here is the link for the docs https://astro-kirsty.github.io/gammapy-docs-preview/

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.

Hi @Astro-Kirsty ! Looking at your generated docs page, the instructions on the "most common way" ie, https://docs.gammapy.org/dev/getting-started/index.html#quickstart-setup are a bit hidden.

Your QuickStart section only shows how to install the data and tutorials https://astro-kirsty.github.io/gammapy-docs-preview/getting-started/index.html#quickstart-setup

I would propose you can simply rename QuickStart to Recommended SetUp, which should make it clearer, no?

environment-dev.yml Outdated Show resolved Hide resolved
@Astro-Kirsty
Copy link
Member Author

Astro-Kirsty commented Jan 17, 2024

Thanks @AtreyeeS, I adjusted it

cgalelli
cgalelli previously approved these changes Jan 18, 2024
Copy link
Contributor

@cgalelli cgalelli left a comment

Choose a reason for hiding this comment

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

Thanks @Astro-Kirsty . The page seems intuitive and easy to read for me now. No further comments from me

Astro-Kirsty and others added 4 commits January 24, 2024 17:35
Signed-off-by: Astro-Kirsty <AstroKirsty@gmail.com>
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
Signed-off-by: Astro-Kirsty <AstroKirsty@gmail.com>
@Astro-Kirsty Astro-Kirsty self-assigned this Jan 24, 2024
@Astro-Kirsty Astro-Kirsty added this to the 1.2 milestone Jan 25, 2024
Signed-off-by: Astro-Kirsty <AstroKirsty@gmail.com>
Signed-off-by: Astro-Kirsty <AstroKirsty@gmail.com>
MRegeard
MRegeard previously approved these changes Jan 25, 2024
Copy link
Member

@MRegeard MRegeard left a comment

Choose a reason for hiding this comment

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

Thanks @Astro-Kirsty, this looks good!

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 @Astro-Kirsty . I am not sure I understand why you removed the instructions for the creation of the environment in the recommended setup. It is no longer visible from the main page then, no?

@Astro-Kirsty
Copy link
Member Author

It was according to #4838.
One of the reasons was because the page seemed too confusing for beginners to gammapy. As there is a 'Virtual Environments' section we thought it best to leave this out, especially because the conda install would install the latest version of gammapy.

If you think it would be best to change it back, let us know!

@bkhelifi
Copy link
Member

Also, there is no mention of mamba. Is it intended?

@adonath
Copy link
Member

adonath commented Jan 27, 2024

Also, there is no mention of mamba. Is it intended?

The lates versions of conda include the mamba solver. I'm not sure it is needed anymore.

@Astro-Kirsty
Copy link
Member Author

mamba is not mentioned on the initial page but is mentioned in "installation" where a full guide of how to install is included.

@Astro-Kirsty
Copy link
Member Author

What is the current opinion here? Can we merge?
@bkhelifi @cgalelli @MRegeard @registerrier @AtreyeeS

@cgalelli
Copy link
Contributor

What would be the conflicts with #5113 ?

@Astro-Kirsty
Copy link
Member Author

Astro-Kirsty commented Feb 26, 2024

No major conflict. That changes the 'Installation' description, which can be adjusted once this PR has been merged.
Or @bkhelifi can commit his changes to this PR?

@AtreyeeS
Copy link
Member

We discussed this PR during one of the last dev calls, but were not sure if the present preview of the docs on your GitHub was the latest version. Reviewing is a bit difficult without seeing how it renders finally.
Can you please share a link to the latest build, @Astro-Kirsty ?

@Astro-Kirsty
Copy link
Member Author

I wouldn't have been in the call.
Present preview is the correct one, see here https://astro-kirsty.github.io/gammapy-docs-preview/

@MRegeard
Copy link
Member

Hi @Astro-Kirsty,
Should we put at least these two lines in the Recommended setup of the getting started pages :

$ curl -O https://gammapy.org/download/install/gammapy-X.Y.Z-environment.yml
$ conda env create -f gammapy-X.Y.Z-environment.yml

and redirect user to the virtual environment page for further informations ?

Because currently this part of the page only shows how to download the tutorials and datasets, and furthermore assume a conda environment (without showing how to create one).

I think installation with in an environment should appear in this page !

@Astro-Kirsty
Copy link
Member Author

Good idea @MRegeard!
I will adapt accordingly

Signed-off-by: Astro-Kirsty <AstroKirsty@gmail.com>
AtreyeeS
AtreyeeS previously approved these changes Feb 27, 2024
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.

This looks ok from my side (please see inline question about specific instructions to windows users)
@adonath can you comment once if the instructions about conda environments are okay?

docs/getting-started/quickstart.rst Outdated Show resolved Hide resolved
Copy link
Member

@adonath adonath left a comment

Choose a reason for hiding this comment

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

Thanks @Astro-Kirsty, I have left some comments.

docs/getting-started/index.rst Outdated Show resolved Hide resolved
docs/getting-started/quickstart.rst Outdated Show resolved Hide resolved
docs/getting-started/quickstart.rst Show resolved Hide resolved
docs/getting-started/quickstart.rst Show resolved Hide resolved
Signed-off-by: Astro-Kirsty <AstroKirsty@gmail.com>
Signed-off-by: Astro-Kirsty <AstroKirsty@gmail.com>
@Astro-Kirsty
Copy link
Member Author

This PR should be ready for final review with changes as per the dev call

Copy link
Contributor

@cgalelli cgalelli left a comment

Choose a reason for hiding this comment

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

Thanks @Astro-Kirsty this final version looks nice to me

Copy link
Member

@adonath adonath left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Astro-Kirsty, no further comments from my side!

Copy link
Member

@MRegeard MRegeard 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 very much @Astro-Kirsty !

@Astro-Kirsty Astro-Kirsty merged commit 2a638ff into gammapy:main Mar 11, 2024
15 of 16 checks passed
@Astro-Kirsty Astro-Kirsty deleted the adapt_starting_guide branch March 15, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review/Idea for the documentation getting started page
7 participants