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

SPdist VAC Review (DR1) #18

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

SPdist VAC Review (DR1) #18

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: Guillaume Thomas
1 catalog file (fits), 1 README,

Initial Checks:

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

Initial Notes:

  • The FITS header name "Joined" is not very descriptive, could you please change it to something more informative? In addition, we prefer extension names to be in ALLCAPS for consistency, so please pick a name in ALLCAPS
  • Similarly to above, some column names are not in ALLCAPS (e.g. Qflag_input, Flag_good), can you please update all of your column names to be in ALLCAPS?
  • Please include a contact person in the README, similarly to a "corresponding author" for a paper.
@dylanagreen dylanagreen self-assigned this Dec 30, 2024
@GFThomas
Copy link

Dear Dylan, I did the adjustments you asked. I also added comments to the different columns directly to the fits file. Let me know if you need any other changes

@dylanagreen
Copy link
Author

dylanagreen commented Jan 13, 2025

Hi @GFThomas, thanks for doing this. I've taken a look at the updated version of the catalog and things are looking much better.

You do have some nans, or what appears to be largely empty rows. For example, CAT_ID == 1, is an array with three values (CAT_ID, FLAG_GOOD, and QFLAG_INPUT) and the rest of the values masked (or non existent). Is it necessary to keep rows like this in the full catalog, since they hold no useful data, it seems? If it is absolutely necessary, can you justify why, and include some information in the README about rows like this?

You also have a hidden .ipnyb_checkpoints folder in the catalog directory, can you remove it please?

@GFThomas
Copy link

Hi @dylanagreen,

I removed the .ipnyb_checkpoints folder.

Regarding the presence of the empty rows is explained by the fact that afte discussion of people using my distance catalogue within DESI-MWS, they found it was easier to have these line present to cross-match it with the main MWS catalogue, since some stars have repeated observations/entries with similar TARGET_ID. This is why these lines are present.

@dylanagreen
Copy link
Author

Hi @GFThomas , thanks for doing this! The VAC is looking good. Just a few last things before we move to final review:

1

"Regarding the presence of the empty rows is explained by the fact that afte discussion of people using my distance catalogue within DESI-MWS, they found it was easier to have these line present to cross-match it with the main MWS catalogue, since some stars have repeated observations/entries with similar TARGET_ID. This is why these lines are present."

Can you please add this description somewhere to the VAC README, for documentation purposes? e.g. something along the lines of "empty rows are included to assist with row matching to the main MWS survey catalog" somewhere in the VAC description

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 spdist or mws-spdist.

Since you do not have a version number encoded in your path, we will release this VAC as v1.0. Is that ok?

4

Do the numbers at the end of the filename (240613) have any significance?

@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