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

Functions to automatically generate reader table for documentation #2108

Merged
merged 10 commits into from Jul 26, 2022

Conversation

BENR0
Copy link
Collaborator

@BENR0 BENR0 commented May 11, 2022

This is a first draft for a feature mentioned in #2096 to automatically generate the reader table in the documentation from the reader yaml config files.

This adds new key words to the reader yaml file (i.e. status and supports_fsspec).
Currently I have added these key words to all the readers which already support fsspec for demonstration purposes. I would add the relevant keywords to all readers yaml files when this draft gets positive feedback.

Other todos are to integrate this into the documentation building process maybe by customizing the makefile to run a python script which writes the table to a rst file which then can be included in the documentation.

  • Tests added
  • Fully documented

Copy link
Member

@djhoese djhoese 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 getting this started! Thanks so much. I had a couple comments, but otherwise I'm excited to see where you take this.

doc/source/dev_guide/custom_reader.rst Show resolved Hide resolved
file format. This can be multiline if formatted properly in YAML (see
example below).
status
The status of the reader (e.g. Nominal, Beta, etc.)
Copy link
Member

Choose a reason for hiding this comment

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

Should we be clear about what options should be used here? Maybe limit it to a set of 3 or 4?

  • Nominal
  • Beta
  • Alpha

Not sure we need something more than those. Other ideas?

Copy link
Collaborator Author

@BENR0 BENR0 May 12, 2022

Choose a reason for hiding this comment

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

Will add Alpha. But in the old table there sometimes are comments for status other than the suggested three (for example the seviri_l1b_nc reader says that HRV is currently not supported. I don't know if such comments should be in a different column or should be allowed in the status column?

Copy link
Member

Choose a reason for hiding this comment

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

...yeah that's difficult to say what is best. It makes the most sense in status, but it also seems ugly and prevents that column from staying small.

Copy link
Collaborator Author

@BENR0 BENR0 May 24, 2022

Choose a reason for hiding this comment

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

@djhoese so I kept the comments in the status column for now since only a few of the readers had status comments and it felt overkill to have a extra column which is mostly empty. It looks like this now:

I added questionmarks for readers for which the status was unclear for me (see my comment below)

satpy/utils.py Outdated Show resolved Hide resolved
@BENR0
Copy link
Collaborator Author

BENR0 commented May 13, 2022

I updated all the reader config files. Additionally I added https://datatables.net java script which makes the reader table search- and sortable (or any table for that matter if :class: datatable is added to the table directive). Since there are so many readers available already I thought that might be a nice feature.

Apart from that there are a couple of readers which were not present in the "old" table so I did not know the status of these:

  • nwcsaf-msg2013-hdf5
  • cmsaf-claas2_l2_nc
  • iasi_l2
  • safe_sar_l2_ocn
  • slstr_l2
  • scatsat1_l2b
  • seviri_l1b_icare
  • avhrr_l1c_eum_gac_fdr_nc
  • ascat_l2_soilmoisture_bufr

@BENR0
Copy link
Collaborator Author

BENR0 commented Jul 1, 2022

@djhoese @pnuu @sfinkens I added the status information for the readers in question based on the slack discussion. So in addition to alpha, beta and nominal there is now defunct. I was thinking that maybe it would be good to "explain" each status above or below the reader table so users which might not be so familiar with these terms know what to expect. At least for the defunct status I think it would be good to add a little footnote. Next to the explanation what it means, this opens up the opportunity to encourage users to work on them and make a PR.

@djhoese
Copy link
Member

djhoese commented Jul 1, 2022

@BENR0 Thanks. That sounds like a good idea.

@BENR0
Copy link
Collaborator Author

BENR0 commented Jul 6, 2022

@djhoese I added some hints regarding the status below the table. Please have a look at it to see if this agrees with how you guys think about the different status meanings.

@djhoese
Copy link
Member

djhoese commented Jul 6, 2022

@BENR0 could you merge current pytroll main with your branch? It looks like your fork's main branch is very old and even has appveyor configuration files in it which we haven't used for a long time.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

The descriptions look good to me. I wasn't initially a fan of nominal readers "most likely no new features will be introduced", but realized that that isn't incorrect.

doc/source/reader_table.py Outdated Show resolved Hide resolved
Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>
@djhoese djhoese added documentation component:readers cleanup Code cleanup but otherwise no change in functionality labels Jul 6, 2022
@djhoese
Copy link
Member

djhoese commented Jul 6, 2022

@BENR0 once the merge conflicts are fixed, is this PR ready for full review and merge?

@BENR0
Copy link
Collaborator Author

BENR0 commented Jul 6, 2022

Yes I would think so if there are no other suggestions.

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #2108 (76f67a1) into main (bafe54e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2108   +/-   ##
=======================================
  Coverage   93.88%   93.88%           
=======================================
  Files         283      283           
  Lines       43093    43093           
=======================================
  Hits        40458    40458           
  Misses       2635     2635           
Flag Coverage Δ
behaviourtests 4.78% <ø> (ø)
unittests 94.53% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bafe54e...76f67a1. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.48% when pulling 76f67a1 on BENR0:feat_automatic_reader_table into bafe54e on pytroll:main.

@BENR0 BENR0 marked this pull request as ready for review July 6, 2022 17:07
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot, a dynamically generated reader table makes our lifes much easier!

@mraspaud mraspaud merged commit 929a258 into pytroll:main Jul 26, 2022
@BENR0
Copy link
Collaborator Author

BENR0 commented Jul 26, 2022

@mraspaud while using the table this morning I found some typos. Nothing serious but should I fix them and push again?

@mraspaud
Copy link
Member

@BENR0 sure, just make a PR, it should be merged quickly :)

@BENR0 BENR0 mentioned this pull request Jul 26, 2022
@BENR0 BENR0 deleted the feat_automatic_reader_table branch July 26, 2022 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code cleanup but otherwise no change in functionality component:readers documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants