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

renaming files in dev notes #231

Merged
merged 43 commits into from
Sep 27, 2024
Merged

renaming files in dev notes #231

merged 43 commits into from
Sep 27, 2024

Conversation

BalzaniEdoardo
Copy link
Collaborator

Reordered the dev notes in a more meaningful way. Corrected the leftover RecurrentGLM class.

@BalzaniEdoardo BalzaniEdoardo changed the title renaming files renaming files in dev notes Sep 24, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.94%. Comparing base (32132b1) to head (5c9adb2).
Report is 291 commits behind head on development.

Additional details and impacted files
@@               Coverage Diff                @@
##           development     #231       +/-   ##
================================================
- Coverage        97.30%   75.94%   -21.36%     
================================================
  Files               18       24        +6     
  Lines             1669     2353      +684     
================================================
+ Hits              1624     1787      +163     
- Misses              45      566      +521     

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

@sjvenditto
Copy link
Collaborator

Some of the page links were broken with the new page numbers, so I went through and fixed them

@BalzaniEdoardo
Copy link
Collaborator Author

I finally took care of the broken links issue.
Now we have a post_build job defined in the .readthedocs.yml that runs html-proofer on the built website.
Any internal or external (404) broken links will result in the build to fail;

I am also waiting that mkdocs-gallery merges my PR, then we will be able to switch to a "strict" build for mkdocs. This mode checks only the internal links, but doesn't check the external, so we will need to keep the htmlproofer call.

Anyways, htmlproofer is stricter than mkdocs (doesn't allow links to ".md" for example, while mkdocs does), and checks even the autogenerated links (since it goes through the html); I really like the tool, we should keep rely on it

Copy link
Collaborator

@sjvenditto sjvenditto left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Member

@billbrod billbrod left a comment

Choose a reason for hiding this comment

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

  • htmlproofer looks great! It's this tool, right? I've been meaning to add something like this to plenoptic
  • Can you write something in this PR about why you're invoking it the way you are? I'm confused about calling on everything but the reference and then just the reference
  • I see you removed the .md from some of the links. That file extension was showing up in the built html? I was pretty sure that .md got converted to html (or the relevant folder), but maybe that's how sphinx does it and I'm confused
  • Can you write a description of what the changes you made to gen_ref_pages.py do?
  • I see you changed how the footnote / references work in only one place. Why only in simulation.difference_of_gammas (and not, e.g., basis.RaisedCosineBasisLog (which seems to work fine)?
  • Additionally, I don't like the HTML in the docstring that this change requires -- it makes the docstring much harder to read from ipython / jupyter / IDE, and will probably confuse our users

@BalzaniEdoardo
Copy link
Collaborator Author

  • htmlproofer looks great! It's this tool, right? I've been meaning to add something like this to plenoptic

Yes, that's the tool, it works great!

  • Can you write something in this PR about why you're invoking it the way you are? I'm confused about calling on everything but the reference and then just the reference

The issue was that htmlproofer is quite opinionated about how the html section should be formatted. And for specific paragraphs it requires an href to be specified. However we are not generating the html for the reference, mkdocstrings is doing that, and doesn't format the html that way. (The pages still renders well buy they have the href field missing in some places) This causes the htmlproofer to fail without a flag to alle missing href.

For the rest of our html pages however we can be stricter, this is why I have to calls to htmlproofer.

  • I see you removed the .md from some of the links. That file extension was showing up in the built html? I was pretty sure that .md got converted to html (or the relevant folder), but maybe that's how sphinx does it and I'm confused

This Is again for the htmlproofer. The links would work ok even with the ".MD" extension, but the html proofer thinks that the link with the extension are wrong because those MD files will are converted to HTML as a folder with the same name of the file, containing an index.html, so there is no "site/path_to_page/scriptname.md" but a "site/path_to_page/scriptname/index.html" instead. (so you can set the link to that folder and to pass the htmlproofer check)

  • Can you write a description of what the changes you made to gen_ref_pages.py do?

Will do!

  • I see you changed how the footnote / references work in only one place. Why only in simulation.difference_of_gammas (and not, e.g., basis.RaisedCosineBasisLog (which seems to work fine)?

This one was broken, so I fixed it. Then I tried to have the reference formatted as numpydoc (without the section header tag) but I did not think that it would be rendered badly as docstrings

  • Additionally, I don't like the HTML in the docstring that this change requires -- it makes the docstring much harder to read from ipython / jupyter / IDE, and will probably confuse our users

Good point , I did not think about that

@BalzaniEdoardo
Copy link
Collaborator Author

BalzaniEdoardo commented Sep 27, 2024

Changes to gen_ref_pages.py

This PR edited the configuration script for the automatic generation of the documentation page.

After we introduced a hierarchy in the folder structure, i.e. sub-modules likesrc/nemos/fetch, the gen_ref_pages.py script as was originally structured generated broken links as well as did not reflect well the hierarchical structure of the modules.

Edits

  • Tuple of module to be skipped in the docs references, including the private modules, and the styles (since it only contains the mpl style file).
  • Simplified logic for excluding modules, moved the logic into a skip_module function that processes the path and checks if a script/module belongs to the one excluded.
  • Add a function filter_nav for filtering the mkdocs the auto-generated navigation iterator (this is the iterator used to build the structure of the literate-navigation page). This filtering function makes sure that all the skipped module are also not listed in the navigation index.
  • Modified how the navigation index for reference/nemos/index.md is generated, so that it can capture the module hierarchy correctly.

Copy link
Member

@billbrod billbrod left a comment

Choose a reason for hiding this comment

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

Sounds good, thanks for adding the clarification!

For the difference between the reference/ pages and the rest, it's weird that mkdocs uses a tags in that way

@BalzaniEdoardo BalzaniEdoardo merged commit c030cfd into development Sep 27, 2024
11 checks passed
@BalzaniEdoardo BalzaniEdoardo deleted the fix_notes branch September 27, 2024 19:49
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.

4 participants