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

Auto-generate tables for the wavelength calibration documentation #1802

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

kbwestfall
Copy link
Collaborator

As titled.

This is largely a documentation PR. There are some changes to how datetime is imported because I realized that datetime.datetime.UTC has never existed (but datetime.UTC does), meaning the try blocks used to import __UTC__ were always faulting (not just for python <=3.10).

As a team, we need to fill in the new pypeit/data/arc_lines/reid_arxiv/summary.txt file!

@codecov-commenter
Copy link

codecov-commenter commented May 6, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 38.00%. Comparing base (081f2f4) to head (824c6f2).

Files with missing lines Patch % Lines
pypeit/setup_gui/controller.py 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1802      +/-   ##
===========================================
- Coverage    38.02%   38.00%   -0.03%     
===========================================
  Files          211      211              
  Lines        48975    48975              
===========================================
- Hits         18625    18614      -11     
- Misses       30350    30361      +11     

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

@tbowers7 tbowers7 linked an issue Jun 13, 2024 that may be closed by this pull request
@tbowers7 tbowers7 self-requested a review June 17, 2024 19:44
	modified:   doc/scripts/build_wvcal_tbl_rst.py
Copy link
Collaborator

@tbowers7 tbowers7 left a comment

Choose a reason for hiding this comment

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

Nice and very needed PR. Am approving, but think about this issue:

My main concern is over maintaining the summary table of information about the reid_arxiv files. I would suggest adding code in the main repo that includes this information in the reid_arxiv files when created, and otherwise hand-adding the same to all the existing files. In this way, no one would need to maintain the summary.txt file, and the .rst table would be automagically generated each time with the files in the directory.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's going to be a bear of a job filling this in the first time. Can't we pull the instrument and disperser from the files when building this table in the first place?

I like the checking logic added to build_wvcal_tbl_rst.py to ensure a 1:1 match between the directory listing and this file, but I worry that the maintenance of this monstrosity will fall to @kbwestfall when he preps the docs for each release.

While it may be a bit of a headache to do, could we add the necessary metadata to each reid_arxiv file (by hand, if necessary, but automatically for any new ones) so that this table can be generated automagically?


def write_reid_arxiv_table(output_root):

# TODO: Pull wavelength range (and resolution?) from files
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment about this on the summary.txt file.

Comment on lines +75 to +86
# Check that all the files are in the table and vice versa
names = numpy.array([f.name for f in files])
indx = numpy.isin(names, tbl['File'])
if not numpy.all(indx):
raise ValueError('Files found in reid_arxiv directory that are *not* in the summary.txt '
f'file. Include summary entries for the following (or delete them): '
f'{names[numpy.logical_not(indx)]}')
indx = numpy.isin(tbl['File'], names)
if not numpy.all(indx):
raise ValueError('Files found in the summary.txt file that are *not* in the reid_arxiv '
'directory. Remove summary entries for the following: '
f'{tbl["File"][numpy.logical_not(indx)]}')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice check!

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.

Out-of-date tables in wavelength-calibration docs
3 participants