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

74 update generated docs #73

Merged
merged 17 commits into from
Nov 7, 2024
Merged

74 update generated docs #73

merged 17 commits into from
Nov 7, 2024

Conversation

StephanieKemna
Copy link
Collaborator

@StephanieKemna StephanieKemna commented Nov 1, 2024

Description

Resolves #74

Cleaning the docs; remove stuff that seems unnecessary, add missing pages, add missing info.

How Has This Been Tested?

  • in docs: make html
  • start build\html\index.html

Screenshots (if appropriate)

Developer Checklist (before requesting PR review)

  • My code follows the style guidelines of this project
  • My changes generate no new warnings
  • [n/a] Any dependent changes have been merged and published in downstream modules

I have:

  • commented my code, particularly in hard-to-understand areas
  • performed a self-review of my own code
  • not committed unnecessary formatting changes, thereby occluding the actual changes (e.g. change of tab spacing, eol, etc.)
  • made corresponding changes to the documentation
  • added change to CHANGELOG.md
  • [n/a] added tests that prove my fix is effective or that my feature works (for core features)

Reviewer checklist

I have:

  • have performed a review of the code
  • tested that the software still works as expected
  • checked updates to documentation
  • checked that the CHANGELOG is updated

@StephanieKemna StephanieKemna self-assigned this Nov 1, 2024
@StephanieKemna StephanieKemna added the documentation Improvements or additions to documentation label Nov 1, 2024
@StephanieKemna
Copy link
Collaborator Author

@ClaasRostock can you have a look at this? wondering whether there was any point to the automodule rst files - they did not seem to generate anything good, so I have removed them. But maybe they should've been set up properly.
There are still a bunch of warnings w.r.t. duplicate stuff, example:
..\src\mlfmu\types\onnx_model.py:docstring of mlfmu.types.onnx_model.ONNXModel.state_size:1: WARNING: duplicate object description of mlfmu.types.onnx_model.ONNXModel.state_size, other instance in _autosummary/mlfmu.types.onnx_model.ONNXModel, use :no-index: for one of them
which I think is due to the autosummary stuff. Not sure if this needs to be resolved or we always have this?

I found some online docs saying this can happen if you have multiple automodule - but I removed automodule rst files, but maybe autosummary creates that?

Do the docs, as I have made them now, look alright? Anything we are missing and should add?

@StephanieKemna StephanieKemna marked this pull request as draft November 1, 2024 21:11
@ClaasRostock
Copy link
Collaborator

Hi Stephanie,

the autosummary Sphinx extension was introduced by David @davidhjp01 in the TDR_rax repo, and honestly I just applied that change also in MLFMU without thinking much about it.
So, it seems now has come a time to look at it at more detail :-).
Let's do so and look into it next week. Maybe you, @davidhjp01 , have any additional input? Otherwise let's just check how we best get the docs build, and if autosummary turns out to stand in way by any reason, then we should be able to also remove it again. Or learn how we can make it do what we want it to :-)

@StephanieKemna
Copy link
Collaborator Author

Hi Stephanie,

the autosummary Sphinx extension was introduced by David @davidhjp01 in the TDR_rax repo, and honestly I just applied that change also in MLFMU without thinking much about it. So, it seems now has come a time to look at it at more detail :-). Let's do so and look into it next week. Maybe you, @davidhjp01 , have any additional input? Otherwise let's just check how we best get the docs build, and if autosummary turns out to stand in way by any reason, then we should be able to also remove it again. Or learn how we can make it do what we want it to :-)

The autosummary is giving warnings, but it is the main source of documentation now. That seems to work nicely.
The automodule stuff did not autogenerate a bunch of documentation in the same fashion, so I removed that. Since I figured we did not need both, and the autosummary docs seemed sufficient to me. But I don't know if that's what the docs 'usually' look like and if we consider it sufficient or not.

@davidhjp01
Copy link
Collaborator

Just had a brief look, try to remove Methods section for class declarations. Seems we do not need it and it conflicts with the docstrings of corresponding methods.

@StephanieKemna
Copy link
Collaborator Author

StephanieKemna commented Nov 4, 2024 via email

…clude for API documentation, instead of auto-documenting everything in the package root.
…e auto-generation of separet stub files for attributes, function, methods and extension. (-> Auto-generate separate stub files only for classes.)
Copy link
Collaborator

@ClaasRostock ClaasRostock left a comment

Choose a reason for hiding this comment

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

made some proposals in a separate PR (meant to be merged into this one, if ok with you)

@ClaasRostock
Copy link
Collaborator

Ah, one more thing: When applying the changes proposed in my PR, you might need to delete all .rst files in your docs/source/_autosummary folder.

It seems that rebuilding the docs (via ./docs/make.bat html) does create any new rst files in that directory, but does not delete any .rst files which are orphaned.

… that class documentation gets generated twice (resolves also the related warnings).
…" to "groupwise", to be in sync with how autosummary orders the members in the respective tables.
… where applicable, to match the formats that Sphinx can expects and can introspect.
@ClaasRostock
Copy link
Collaborator

@ClaasRostock can you have a look at this? wondering whether there was any point to the automodule rst files - they did not seem to generate anything good, so I have removed them. But maybe they should've been set up properly. There are still a bunch of warnings w.r.t. duplicate stuff, example: ..\src\mlfmu\types\onnx_model.py:docstring of mlfmu.types.onnx_model.ONNXModel.state_size:1: WARNING: duplicate object description of mlfmu.types.onnx_model.ONNXModel.state_size, other instance in _autosummary/mlfmu.types.onnx_model.ONNXModel, use :no-index: for one of them which I think is due to the autosummary stuff. Not sure if this needs to be resolved or we always have this?

I found some online docs saying this can happen if you have multiple automodule - but I removed automodule rst files, but maybe autosummary creates that?

Do the docs, as I have made them now, look alright? Anything we are missing and should add?

@StephanieKemna @davidhjp01
I found a solution to above problem. Solved with latest commit in PR #74 , branch 74-update-generated-docs-review-claas (51a291e).
If you merge PR #74 into 74-update-generated-docs, then this PR #73 is from my point of view good to go.

@StephanieKemna
Copy link
Collaborator Author

Remaining tasks: #72 (comment)

@StephanieKemna StephanieKemna marked this pull request as ready for review November 7, 2024 10:25
@StephanieKemna StephanieKemna merged commit 97ce20e into main Nov 7, 2024
15 checks passed
@StephanieKemna StephanieKemna deleted the 74-update-generated-docs branch November 8, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants