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

MWS Stellar Distances VAC Review (DR1) #20

Open
3 of 4 tasks
dylanagreen opened this issue Dec 30, 2024 · 4 comments
Open
3 of 4 tasks

MWS Stellar Distances VAC Review (DR1) #20

dylanagreen opened this issue Dec 30, 2024 · 4 comments
Assignees
Labels
vac review Issues pertaining to VAC reviews

Comments

@dylanagreen
Copy link

dylanagreen commented Dec 30, 2024

Contact Person: Wenting Wang / Songting Li
1 catalog file (fits), 5 catalog files (csv), 1 catalog file (npy?), 1 README, 1 jupyter notebook

Initial Checks:

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

Initial Notes (iron-yr1-v1.0.fits):

  • Please include an extensinon name in the fits file, so that the HDU can be accessed by name instead of index
  • Some column names are not in ALLCAPS, please update these to be ALLCAPS for consistency and ease of use

Initial notes (external_dist/*.csv):

  • Please update column names to be in ALLCAPS, if possible

Initial notes (dustapprox-main/*.csv):

  • Please update column names to be in ALLCAPS, if possible
  • There's an .ipynb_checkpoints folder in this directory that looks like it got overlooked, can you please remove it?

Initial notes (mask.npy):

  • We prefer not to release files in unusual or language specific file formats like .npy files. Is there any other file format that would be amenable to the data in this file? e.g. a csv file, an hdf5 archive, or a json dump?

Initial notes (quick_start.ipynb):

  • This notebook is looking good, but many of the file path references are to private locations (e.g. Songting's private cfs, the current draft directory of the VAC instead of the later public directory). Where possible can you please update these to be to public locations? If there are some that cannot be updated to refer to public locations, then we will have to remove those cells. The reason for this is that on principle we want any released jupyter notebooks to be runnable by the public end user who may download the VAC.

Some other notes:

  • The file gaia_reddening_law.py is not mentioned in the README, is this file intended to be released as part of the VAC? If so, please include it in the README
  • When possible, please update the README to have a data model for all data files (if some files share the same data model, it is ok to only include it once in the README, but it is still necessary to have a data model represented for every file)
  • When you get a chance, please send a paragraph or two summary of the VAC to include in the DR1 paper.
@dylanagreen dylanagreen self-assigned this Dec 30, 2024
@wenting91
Copy link

A new version of the SpecDis VAC catalog is at: /global/cfs/cdirs/desicollab/science/mws/value_added/distances/spectro_dist/v2.0 , with the above improvements incorporated.

@dylanagreen
Copy link
Author

dylanagreen commented Jan 13, 2025

Hello @wenting91, I've taken a look over the updated catalog and it's looking much better. A couple additional notes:

README:

  • Please add a contact person (akin to a "corresponding author" to a paper)
  • Can you add a brief summary of the VAC to the README, as well as a list of included files?

General comments:

  • Can you send a one to two paragraph summary of the VAC to include in the DR1 paper?
  • You have a couple masked values, for example in the P_COLOUR and NEUIA columns, can you justify why we should include these and if we absolutely need to keep them can you document them in the README somewhere, preferably in the column in which they appear in the data model?
  • You have an invisible .ipynb_checkpoints folder in your directory tree, please remove it before we finalize the VAC.

@SongtingL
Copy link

Hi @dylanagreen, thanks for your comments

README:

  • Please add a contact person (akin to a "corresponding author" to a paper)
    Done
  • Can you add a brief summary of the VAC to the README, as well as a list of included files?
    Done

General comments:

  • Can you send a one to two paragraph summary of the VAC to include in the DR1 paper?
    Yes, we have send a summary paragraph of the VAC before.
  • You have a couple masked values, for example in the P_COLOUR and NEUIA columns, can you justify why we should include these and if we absolutely need to keep them can you document them in the README somewhere, preferably in the column in which they appear in the data model?
    The masked values in this VAC for examples P_COLOUR and NEUIA columns are some ASTROMETRIC parameters from Gaia DR 3 which are used for calculating Zero point correction of Gaia parallax.
  • You have an invisible .ipynb_checkpoints folder in your directory tree, please remove it before we finalize the VAC.
    We have removed it.

@dylanagreen
Copy link
Author

Hi @SongtingL, thanks for doing this! The VAC is looking pretty good. Just a few last things:

1

"The masked values in this VAC for examples P_COLOUR and NEUIA columns are some ASTROMETRIC parameters from Gaia DR 3 which are used for calculating Zero point correction of Gaia parallax."

Can you please add this description somewhere to the VAC README, for documentation purposes? e.g. can you document why they would be masked vs having values?

2

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

3

We need to pick a stubname for this VAC that will be used for on disk reference (e.g. /dr1/stubname/v1.0/), do you have a preference? I suggest some variation of specdis or mws-specdis.

In addition, when we release the VAC, do you want to release the vac as v2.0 (as indicated in the file tree) or will this finalized copy be v1.0?

@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

3 participants