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

Doc and import improvement #47

Merged
merged 7 commits into from
Nov 6, 2022

Conversation

ericpre
Copy link
Member

@ericpre ericpre commented Sep 19, 2022

As discussed in #38. This changes the import and docstring/documentation for the bruker and blockfile format as examples for discussion.

Progress of the PR

  • Change implemented (can be split into several points),
  • update docstring (if appropriate),
  • update user guide (if appropriate),
  • add an changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
  • Check formatting changelog entry in the readthedocs doc build of this PR (link in github checks)
  • add tests,
  • ready for review.

Minimal example of the bug fix or the new feature

from rsciio.msa import api
api.file_reader("your_msa_file.msa")
# Your new feature...

Note that this example can be useful to update the user guide.

@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Base: 83.35% // Head: 83.39% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (d08ae32) compared to base (e641fca).
Patch coverage: 97.29% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
+ Coverage   83.35%   83.39%   +0.04%     
==========================================
  Files          40       43       +3     
  Lines        8337     8348      +11     
  Branches     1896     1896              
==========================================
+ Hits         6949     6962      +13     
+ Misses        913      911       -2     
  Partials      475      475              
Impacted Files Coverage Δ
rsciio/blockfile/_api.py 95.31% <95.65%> (ø)
rsciio/blockfile/__init__.py 100.00% <100.00%> (ø)
rsciio/bruker/__init__.py 100.00% <100.00%> (ø)
rsciio/bruker/_api.py 88.27% <100.00%> (ø)
rsciio/docstrings.py 100.00% <100.00%> (ø)
rsciio/image/api.py 91.56% <0.00%> (+2.40%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

docs/supported_formats/bruker.rst Outdated Show resolved Hide resolved
rsciio/bruker/_api.py Outdated Show resolved Hide resolved
rsciio/bruker/_api.py Outdated Show resolved Hide resolved
rsciio/bruker/_api.py Outdated Show resolved Hide resolved
rsciio/bruker/_api.py Outdated Show resolved Hide resolved
rsciio/blockfile/_api.py Outdated Show resolved Hide resolved
rsciio/blockfile/_api.py Show resolved Hide resolved
docs/supported_formats/bruker.rst Outdated Show resolved Hide resolved
docs/supported_formats/bruker.rst Outdated Show resolved Hide resolved
docs/supported_formats/bruker.rst Outdated Show resolved Hide resolved
@jlaehne
Copy link
Contributor

jlaehne commented Sep 19, 2022

Loading the API documentation in the user guide worked out for bruker, but not for blockfile.

@jlaehne
Copy link
Contributor

jlaehne commented Sep 20, 2022

All other format descriptions consistently have the . in the file abbreviations -- we can change it everywhere when going through the formats, but we should be consistent.

@ericpre ericpre force-pushed the doc_and_import_improvement branch 3 times, most recently from 3423eb2 to 068732f Compare September 20, 2022 21:17
@jlaehne
Copy link
Contributor

jlaehne commented Sep 20, 2022

A subsection header for the API part of the format pages would be nice, e.g. API documentation or API parameters or Provided API functions

@ericpre
Copy link
Member Author

ericpre commented Sep 22, 2022

Loading the API documentation in the user guide worked out for bruker, but not for blockfile.

scikit-image was missing from the build and as it is a requirement for the blockfile plugin, the import of this module will fail.

All other format descriptions consistently have the . in the file abbreviations -- we can change it everywhere when going through the formats, but we should be consistent.

Ok, if this is done consistently in other format, I will revert this change. I am not sure if we need the . but we should consider it in a separate PR.

A subsection header for the API part of the format pages would be nice, e.g. API documentation or API parameters or Provided API functions

I started to add one but I find it a bit unnecessary/redundant.

@jlaehne
Copy link
Contributor

jlaehne commented Sep 22, 2022

Ok, if this is done consistently in other format, I will revert this change. I am not sure if we need the . but we should consider it in a separate PR.

It was a complete mixture, but in a previous PR on the docs I did it that way. Can live with the other way at well, but should be uniform.

A subsection header for the API part of the format pages would be nice, e.g. API documentation or API parameters or Provided API functions

I started to add one but I find it a bit unnecessary/redundant.

Well, if the initial description is long, it does not hurt to have a link in the right-side menu to jump there. And for a page like the NeXus one, where there are various subsections, I would find it weird to not have one for the API part.

@ericpre
Copy link
Member Author

ericpre commented Sep 23, 2022

Before continuing this PR, it would good to agree on the main changes:

Public API

  • importing functions have changed from from rsciio.bruker.api import file_reader to from rsciio.bruker import file_reader
  • the public api of a plugin is defined in __init__ and expose only file_reader and file_writer, all other functions would be made private
  • the submodules are private
  • the io plugins registry haven't changed

@francisco-dlp, you implemented the current structure of the plugins, does the simplification of this PR sounds good to you?

Documentation

  • Pull docstring of file_reader/file_write in plugin documentation page to avoid duplication of documentation - in particular for parameters
  • For plugin, the documentation should have an introduction to the format, then the API (sphinx automodule) and some other section where suitable.

@jlaehne
Copy link
Contributor

jlaehne commented Sep 23, 2022

For the procedure to continue, should we do all the readers in this PR (other people could contribute certain readers) or do separate PRs for a number of plugins at a time?

@ericpre
Copy link
Member Author

ericpre commented Sep 24, 2022

I would prefer separate PRs, otherwise the diff preview on github is difficult to use... As several plugins with simple (or related/similar) changes can be put in one single PR, there shouldn't be that many PRs, maybe about 6-7?

@jlaehne
Copy link
Contributor

jlaehne commented Sep 24, 2022

Before continuing this PR, it would good to agree on the main changes:

I agree, a reply from @francisco-dlp would be good though.

@ericpre ericpre added this to the v0.1.0 initial release milestone Oct 16, 2022
@jlaehne
Copy link
Contributor

jlaehne commented Nov 2, 2022

A subsection header for the API part of the format pages would be nice, e.g. API documentation or API parameters or Provided API functions

In my opinion, this PR would just need a suitable header for the pulled in API sections and then we could proceed to implement everything for the other readers as well. I would say API functions is appropriate.

@francisco-dlp
Copy link
Member

LGTM!

@jlaehne jlaehne mentioned this pull request Nov 5, 2022
7 tasks
@ericpre
Copy link
Member Author

ericpre commented Nov 5, 2022

@jlaehne, should we merge this PR to update other plugins documentation in separate PRs?

rsciio/bruker/_api.py Outdated Show resolved Hide resolved
rsciio/blockfile/_api.py Outdated Show resolved Hide resolved
rsciio/blockfile/_api.py Outdated Show resolved Hide resolved
rsciio/bruker/_api.py Show resolved Hide resolved
rsciio/blockfile/_api.py Show resolved Hide resolved
rsciio/bruker/_api.py Show resolved Hide resolved
@jlaehne
Copy link
Contributor

jlaehne commented Nov 5, 2022

Just a few points I realized while working on the first other formats that we should maybe fix before merging. In the meantime, I started working from the back of the alphabet in #59 - the PR needs to be rebased once this one is merged as I included your changes to start out with.

@ericpre ericpre force-pushed the doc_and_import_improvement branch 2 times, most recently from b385661 to 614e71d Compare November 5, 2022 21:52
rsciio/docstrings.py Outdated Show resolved Hide resolved
"""

SIGNAL_DOC = """signal : dict
The signal to save.
Copy link
Contributor

Choose a reason for hiding this comment

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

Dictionary containing the signal object.

Better would be to give more details on the format of the dictionaries, or simply link the API page in the user guide that gives the details on these dictionaries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, I have left it simple for now thinking that we can improve it in another iteration! :)

@jlaehne jlaehne merged commit 1fe89dd into hyperspy:main Nov 6, 2022
@jlaehne
Copy link
Contributor

jlaehne commented Nov 6, 2022

I will update #59 accordingly to reflect the latest changes in this PR.

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.

3 participants