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

Data model for the exposure and tile qa files #132

Merged
merged 4 commits into from Jun 16, 2022
Merged

Conversation

araichoor
Copy link
Contributor

This PR addresses #114.
It modifies the data model for the exposure-qa*fits files and the tile-qa*fits files, as both are very close.

Comments:

  • it would be great if @julienguy could check the column description is correct;
  • for the PETALQA extension, I ve put the definition in the exposure-qa-EXPID.rst, and refer to those in the tile-qa-TILEID-GROUPID.rst;
  • I refer to the fiberassign-TILEID.rst file, which is in the not-yet-branch of the PR Datamodel for fiberassign files #103 (so the building complains the file does not exist).

A built documentation is here: https://data.desi.lbl.gov/desi/users/raichoor/tmpdir/desidatamodel_exptileqa/20220615-v0/html.

@coveralls
Copy link

coveralls commented Jun 16, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling adf170b on exptileqa into df8b1b5 on fiberassign.

@weaverba137
Copy link
Member

The files look fine, but it appears that this assumes that #103 is already merged, when in fact it is not. Would you consider merging this branch into fiberassign rather than main?

@araichoor
Copy link
Contributor Author

Thanks.

That is correct: I've added a comment, which refers to a file which will appear in PR #103.

I could:

  • either remove that comment (no big deal; actually those infos which are propagated from fiberassign are in numerous files, and I think none mention that it comes from fiberassign);
  • or proceed as you suggest (in which case, I d appreciate instructions on how to do that).

Please let me know.

@weaverba137
Copy link
Member

Changing the branch can be done in GitHub itself. Let me test that.

@weaverba137 weaverba137 changed the base branch from main to fiberassign June 16, 2022 16:54
* fiberassign: (23 commits)
  add note about complete rebuild
  update change log
  improve description of FIBER_FRACFLUX_ELG_GFA
  Fix indents and newline to resolve Sphinx issues
  Add file description for exposures-SPECPROD
  Add descriptions for top level exposures and tiles table files
  fix sphinx linking errors
  add intermediate files in DESI_SURVEY
  bugfix: units
  update fiberassign-EXPID to fiberassign-TILEID
  bugfix: typo
  bit more descriptive index
  update deprecated EXPID to TILEID in file name
  more accurate HDU0 description
  bugfix: correct link to fibermap-EXPID
  bugfix: width formatting
  update fiberassign file + index in doc/DESI_SPECTRO_DATA/NIGHT/EXPID
  add index.rst files for the doc/DESI_TARGET/fiberassign/tiles/TILES_VERSION/TILEXX structure
  remove doc/DESI_TARGET/fiberassign/tile-TILEID-FIELDNUM.rst
  rename fiberassign-EXPID.rst to fiberassign-TILEID.rst
  ...
@weaverba137
Copy link
Member

@araichoor, this is ready to merge now. Do you have any further changes?

@araichoor
Copy link
Contributor Author

Thanks a lot for handling that.
I ve no planned further change.
It would be nice to have Julien s check on the column description, but that can surely be done later, after merging.
So I think you can merge.

@weaverba137 weaverba137 merged commit bf9209c into fiberassign Jun 16, 2022
@weaverba137 weaverba137 deleted the exptileqa branch June 16, 2022 17:14
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.

None yet

3 participants