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

tile-based spectra/redshift column updates #145

Merged
merged 3 commits into from Mar 10, 2023
Merged

tile-based spectra/redshift column updates #145

merged 3 commits into from Mar 10, 2023

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Mar 7, 2023

This is the next installment in updates to the data models in DESI_SPECTRO_REDUX/SPECPROD/tiles/GROUPTYPE/TILEID/GROUPID, with columns descriptions updated with

update_column_descriptions -i filename.rst [--inplace --force]

I first ran without --inplace --force to see what it would update, and then updated py/desidatamodel/data/column_descriptions.csv if I thought any pre-existing description was better than what we had in column_descriptions.csv. I then reran with --inplace --force to do the actual updates. Exception: I kept the zmtl ZWARN and ZTILED slightly different from the standard descriptions, since zmtl augments the ZWARN flags from redrock, and the standard ZTILEID description is for the mtl ledgers where there actually can be more than one tile, which doesn't apply to zmtl.

This PR also includes the new option --inplace, which is a convenience option equivalent to providing the same --outfile as --infile (i.e. so you only have to write the input filename once, and don't risk a cut-and-paste error or replacing one datamodel with another).

@sbailey sbailey requested a review from weaverba137 March 7, 2023 18:42
@coveralls
Copy link

coveralls commented Mar 7, 2023

Coverage Status

Coverage: 100.0%. Remained the same when pulling a31e805 on spectra into 180dc7e on main.

Copy link
Member

@weaverba137 weaverba137 left a comment

Choose a reason for hiding this comment

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

Just a minor recommendation to clean up formatting on one file where you replaced some footnotes with a bullet list.

@weaverba137
Copy link
Member

Also, please update the change log. You may want to merge or rebase with main to avoid possible conflicts on that file.

@weaverba137
Copy link
Member

@sbailey, in order to move this along, do you want me to make the requested changes myself?

@sbailey
Copy link
Contributor Author

sbailey commented Mar 10, 2023

@weaverba137 thanks for the offer. I updated the change log and fixed the formatting; the delay was due to also getting clarification about the meaning of some of the columns and footnotes, which I have now also updated. If you can take it from here, I would appreciate that. Thanks.

@weaverba137
Copy link
Member

OK, sounds good.

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