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

BH Mass VAC Review (DR1) #13

Open
2 of 4 tasks
dylanagreen opened this issue Jan 19, 2024 · 2 comments
Open
2 of 4 tasks

BH Mass VAC Review (DR1) #13

dylanagreen opened this issue Jan 19, 2024 · 2 comments
Assignees
Labels
vac review Issues pertaining to VAC reviews

Comments

@dylanagreen
Copy link

Draft directory:/global/cfs/cdirs/desi/users/pzw/v1
1 catalog file, 2 files total

Initial Checks:

  • Includes README
  • Columns in ALLCAPS
  • Extension names in ALLCAPS
  • Files include units

Initial Notes:

  • The extension name "Bhmass" is not all caps, please update it
  • Please include units in both the VAC file as well as the README file.
  • What's the version number on your catalog? The file is labeled v1.1, but the directory level is v1, we should pick one so that they're consistent.
  • No comments on the README at this time, although this might change at a later date.
@dylanagreen dylanagreen self-assigned this Jan 19, 2024
@panzhiwei1997
Copy link

  • I have updated the extension name and the units.
  • The latest version, v1.6, has been uploaded to the directory v1/v1.6. 'v1.6' will be the version number of my catalog.
  • Thank you for your comments. Please let me know if there are any issues or further adjustments needed.

@dylanagreen
Copy link
Author

dylanagreen commented Jan 17, 2025

Hi @panzhiwei1997, thanks for the updates. I have a few more notes before we can move to final review:

1

The catalog file is looking pretty good! Can you please make the following few updates:

  1. Update all column names to be in ALLCAPS where necessary? e.g. LOGMASS_DAS_Pan25 and similar columns
  2. Thanks for adding units! There are a few errors in the units though: for Angstroms us Angstrom instead of \AA and for solar mass you can use solMass instead of M_odot.
  3. The numeric factor of 1e44 on erg/s doesn't parse correctly in astropy but I think this should be fine, I will verify with Ben in final review (when we get there)

2

The README is looking good! I noticed there are a few masked values in the catalog, can you include a justification/description of why some values are masked in the README description, for documentation purposes?

3

We need to decide on a stubname. The stubname will be used for the README file as well as the on-disk location for the vac (e.g. /dr1/stubname/v1.6/). Do you have a preferred stubname? Otherwise I suggest a variation of bhmass

4

I would like to encourage you to include a checksum in your fits file, you can do this easily using astropy: https://docs.astropy.org/en/stable/io/fits/api/tables.html#astropy.io.fits.BinTableHDU.add_checksum

Once we settle these I think we will be close to ready for final review.

@dylanagreen dylanagreen added the vac review Issues pertaining to VAC reviews label Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vac review Issues pertaining to VAC reviews
Projects
None yet
Development

No branches or pull requests

2 participants