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

(towards #2543) Simplify Introduction documentation #2770

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

sergisiso
Copy link
Collaborator

@sergisiso sergisiso commented Nov 8, 2024

  • Added "warnings as errors" in the shpinx CI job

  • Renamed "nemo" to "generic" in tutorials and lib folders.

  • I removed lots of documentation that seem unneeded: two files where not used, some explanation e.g. how to use venvs is better with a link to an authoritative the source, how to install each dependency separately is probably not needed....

  • Merged tutorials and examples sections and rewrote tutorials part

  • I made termcolour and graphviz standard dependencies

  • I started using sphinx tabs for alternative options, see image:

image

Copy link

codecov bot commented Nov 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.88%. Comparing base (d865146) to head (73a3282).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2770      +/-   ##
==========================================
+ Coverage   99.87%   99.88%   +0.01%     
==========================================
  Files         355      355              
  Lines       49324    49321       -3     
==========================================
+ Hits        49260    49263       +3     
+ Misses         64       58       -6     

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

@sergisiso
Copy link
Collaborator Author

@arporter @hiker This is ready for review. A bit unfortunately the linkcheck does not pass because the last commit updated a link to https://github.com/stfc/PSyclone/tree/master/tutorial/practicals/generic which won't exist until itself is merged, but you can check in the previous commit that the tests pass with the warning as errors flags.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Nice job Sergi. Thanks very much for all your work on improving the documentation.
Mostly small things. The only concern I have is that the funky new 'tab' option doesn't render very well in latexpdf but perhaps we can tweak it (or just live with it)?
docs build fine for me (and this is now tested in the CI which is great).

@@ -66,11 +66,18 @@ jobs:
- run: sudo apt-get install -y graphviz doxygen
- run: python -m pip install --upgrade pip
- run: pip install .[doc]
# First we need a first build and launching a server, otherwise the
Copy link
Member

Choose a reason for hiding this comment

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

"...we need to build the docs and then launch a server..."

# Now we can check for warnings and broken links
- run: cd doc/user_guide; make html SPHINXOPTS="-W --keep-going"
- run: cd doc/developer_guide; make html SPHINXOPTS="-W --keep-going"
# - run: cd doc/reference_guide; make html
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a TODO referencing #1280 ?

* The ``psyclone`` :ref:`script <psyclone_command>` is located
in ``<python-base-prefix>/bin`` directory (depending on your Linux
distribution, you may need to add this location to your ``$PATH``).
please see :ref:`Installation section in the Developer Guide <dev-installation>`.
Copy link
Member

Choose a reason for hiding this comment

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

"...see the :ref:...."

expressions as ``fparser`` does not fully parse these (see
`here <https://github.com/pyparsing>`__ for more information).
For more information about using ``pip`` or encapsulating the installation
in its own ``virtual environment`` we recomment reading the
Copy link
Member

Choose a reason for hiding this comment

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

"recommend"


> pip uninstall pyparsing
For more information about how to use Spack we recommend readign the
Copy link
Member

Choose a reason for hiding this comment

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

"reading"


PSyclone provides a hands-on tutorial. The easiest way to follow it is reading
the `Readme files in github <https://github.com/stfc/PSyclone/tree/master/tutorial/practicals>`_.
The tutorial is divided in two sections, a first section that introduces
Copy link
Member

Choose a reason for hiding this comment

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

"...divided into two..."

@@ -749,21 +800,3 @@ For more details see the ``examples/nemo/README.md`` file.
Note that these scripts are here to support the ongoing development of the
NEMO API in PSyclone. They are *not* intended as 'turn-key' solutions but
Copy link
Member

Choose a reason for hiding this comment

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

Please update. Perhaps 'ongoing development of PSyclone to transform NEMO source.'?

@@ -749,21 +800,3 @@ For more details see the ``examples/nemo/README.md`` file.
Note that these scripts are here to support the ongoing development of the
NEMO API in PSyclone. They are *not* intended as 'turn-key' solutions but
as a starting point.
Copy link
Member

Choose a reason for hiding this comment

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

Please also update the other mentions of 'NEMO API' in the SIR and Kernel Data Extraction sections above this one.

@@ -81,10 +81,11 @@ def have_graphviz():
''' Whether or not the system has graphviz installed. Note that this
Copy link
Member

Choose a reason for hiding this comment

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

Please update this docstring - it is now checking that the library is installed (since the bindings are now a mandatory dependency).

@@ -35,6 +35,6 @@

# Top-level Makefile to test all of the NEMO hands-on tutorials.
Copy link
Member

Choose a reason for hiding this comment

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

"hands-on tutorials for transforming generic code." perhaps?

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.

2 participants