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

DESIVAST VAC Review (DR1) #9

Closed
4 tasks done
dylanagreen opened this issue Nov 29, 2023 · 12 comments
Closed
4 tasks done

DESIVAST VAC Review (DR1) #9

dylanagreen opened this issue Nov 29, 2023 · 12 comments
Assignees
Labels
vac review Issues pertaining to VAC reviews

Comments

@dylanagreen
Copy link

dylanagreen commented Nov 29, 2023

Draft directory: /global/cfs/cdirs/desi/users/hrincon/DESIVAST_V1
24 catalog files, 25 total

Initial Checks:

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

Initial Notes:

  • Your FITS files don't have EXTNAMEs, could you please add some to make it easier to access the table by EXTNAME instead of indices?
  • Generally we request that FITS files include column units. You do have the units documented in the README, would it be possible to also propagate those to the fits files themselves?
  • Can you draft a short (~1-2 paragraph) summary of the VAC to include in the DR1 paper? For an example of what this would look like you can look at section 3.3.6 of the EDR paper (https://arxiv.org/abs/2306.06308).
@dylanagreen dylanagreen self-assigned this Nov 29, 2023
@hbrincon
Copy link

hbrincon commented Dec 4, 2023

@dylanagreen Revisions are located in /global/cfs/cdirs/desi/users/hrincon/DESIVAST_V2

The files should now have EXTNAMES and column units. A draft for the one paragraph VAC summary is located in global/cfs/cdirs/desi/users/hrincon/DESIVAST_V2/VAC_description.txt

@dylanagreen
Copy link
Author

@hbrincon I've taken a look over the V2 of the files and they all look pretty good to me. Here's the last of my notes before we move to final review:

1

You have an invisible .ipynb_checkpoints filder in your VAC directory, can you please delete it?

2

Pursuant to our previous emails, 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.0/). Do you have a preferred stubname? Otherwise I suggest a variation of desivast

In addition, on disk your VAC is listed as V2. When we release this VAC publicly, would you like it to be labeled as v2.0, or as v1.0?

3

I would like to encourage you to include a checksum in your fits files, you can do this easily using astropy: https://docs.astropy.org/en/stable/io/fits/api/tables.html#astropy.io.fits.BinTableHDU.add_checksum, although I recognize that you have a lot of files to iterate over to do this.

I think once we square these away we will be close to ready for final review!

@dylanagreen dylanagreen added the vac review Issues pertaining to VAC reviews label Jan 17, 2025
@hbrincon
Copy link

hbrincon commented Feb 3, 2025

Hi @dylanagreen, apologies for not notifying you of this before, but the most recent revisions were located in /global/cfs/cdirs/desi/users/hrincon/DESIVAST_V4 (In response to your second point, I've renamed this folder as DESIVAST_V1). The files are formatted the same way as those you've reviewed, but with updated data.

  1. I've deleted the .ipynb_checkpoints folder. It seems to be recreated whenever I open a .txt file, but I'll continue to delete it when so.
  2. desivast would be a good name. The public release will be V1, and I've renamed the desired folder to DESIVAST_V1 to avoid confusion.
  3. I've added checksums to the fits tables.

Let me know if there's anything else for now. Thanks!

@dylanagreen
Copy link
Author

Hi @hbrincon, thanks for doing this. I think we're pretty close now. In the process of moving your README over to desidatadocs I discovered that some columns are documented as having units but those units do not appear in your files. For example in all your files your documentation (https://vast.readthedocs.io/en/latest/Vsquared_examples.html#output, https://vast.readthedocs.io/en/latest/VoidFinder_examples.html#output) indicate that the columns X, Y, Z, RADIUS have units of Mpc/h but the units do not appear in the files themselves. This is just a sampling of columns, but can you double check that all the columns that units in the documentation have units in the files themselves? I am reasonably sure this is the last thing we need to take care of before a final review. Thanks!

@hbrincon
Copy link

hbrincon commented Feb 4, 2025

Hi @dylanagreen, thanks for catching that. I've gone back and added units where they're missing (I'd wiped them by accident)

@dylanagreen
Copy link
Author

dylanagreen commented Feb 4, 2025

Thanks Hernan. @weaverba137, let's move this to final review with the stubname desivast, and version v1.0.

@weaverba137
Copy link
Member

I see there are two text files, README.txt, and VAC_description_for_DR1_paper.txt. Are both of those being incorporated into the desidatadocs description?

@dylanagreen
Copy link
Author

I haven't added the VAC_description to the README, but if we need to add more detail I'll take the semi-final version from the DR1 paper so we can ignore the one on disk.

@weaverba137
Copy link
Member

OK, I will keep the files for now in the staging area, but they should be removed before moving to the final public area.

@dylanagreen
Copy link
Author

Now that the README is finalized, let's move this to the final location and I'll close this issue off.

@weaverba137
Copy link
Member

OK done.

@hbrincon
Copy link

hbrincon commented Feb 5, 2025

Thanks for taking care of this @dylanagreen. To keep you updated, I noticed that some of the column names on the website that the README links to are outdated (e.g., "flag" should be "void" on https://vast.readthedocs.io/en/latest/VoidFinder_examples.html#output). I've brought the documentation up to date on a branch of the website, and the website itself should be updated soon

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