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 VAC Review (DR1) #21

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

MWS VAC Review (DR1) #21

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

Comments

@dylanagreen
Copy link

Contact Person: Sergey Koposov
1 catalog file (fits), 1 README, variety of files in rv_output and sp_output

Initial Checks:

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

Initial Notes:

  • This looks like it's mainly an update of a previously released EDR VAC. The files look very similar to those in EDR, but with updated values.
  • Can you please add units of degrees to the TARGET_RA and TARGET_DEC columns in mwsall-pix-iron.fits?
@dylanagreen dylanagreen self-assigned this Dec 30, 2024
@segasai
Copy link

segasai commented Jan 6, 2025

Thanks @dylanagreen
I've looked into TARGET_RA, TARGET_DEC the reason why they don't have units is because the FIBERMAP does not have them (and I just copy those verbatim).
I will add units to ra/dec in the main 'concatenated' tables mwsall. (the individual hpx tables will stay as they are)

@dylanagreen
Copy link
Author

Thanks @segasai, that is a good point about those columns being copied directly from the FIBERMAP. For now let's leave the columns without units to match the data FIBERMAP. Given that let's move this to final review @weaverba137

@weaverba137
Copy link
Member

OK, there are some potentially serious issues with this one.

  1. The data layout in rv_output and sp_output does not match EDR. Are those directories meant to be rearranged?
  2. Subdirectories in rv_output and sp_output contain symlinks that currently point outside of the dr1 data tree. What is meant to be done with these? There are some hints in EDR, but I need to be certain of what to do here.

@segasai
Copy link

segasai commented Jan 16, 2025

  1. Yes, it was decided that it is a more reasonable arrangement. (and the data-model correctly reflects the change).
  2. On the symlinks, I can replace them with actual files. I just kept them for the time-being, while things are being checked/finalised.

@weaverba137
Copy link
Member

@segasai, thank you. I will copy the files by following the symlinks, so there's no need to do anything additional to your copy.

@weaverba137
Copy link
Member

@segasai, I've completed the copies of the healpix directories. While doing that, I found a number of what appear to be extraneous files in sp_output/230211/healpix. Here is a sample:

healpix/special/other/slurm-5963650.out
healpix/special/other/27/2709/coadd-special-other-2709.log_10
healpix/special/other/27/2709/input.lst-coadd-special-other-2709_07
healpix/special/other/27/2709/input.lst-coadd-special-other-2709_01
healpix/special/other/27/2709/input.lst-coadd-special-other-2709_03
healpix/special/other/27/2709/coadd-special-other-2709.log_01
healpix/special/other/27/2709/coadd-special-other-2709.log_08
healpix/special/other/27/2709/input.lst-coadd-special-other-2709_10
healpix/special/other/27/2709/input.lst-coadd-special-other-2709_02
healpix/special/other/27/2709/coadd-special-other-2709.opf
healpix/special/other/27/2709/coadd-special-other-2709.log_02
healpix/special/other/27/2709/coadd-special-other-2709.log_05
healpix/special/other/27/2709/coadd-special-other-2709.log_09
healpix/special/other/27/2709/coadd-special-other-2709.log_04
healpix/special/other/27/2709/input.lst-coadd-special-other-2709_04
healpix/special/other/27/2709/input.lst-coadd-special-other-2709_05
healpix/special/other/27/2709/input.lst-coadd-special-other-2709_08
healpix/special/other/27/2709/input.lst-coadd-special-other-2709_06
healpix/special/other/27/2709/coadd-special-other-2709.log_03
healpix/special/other/27/2709/coadd-special-other-2709.log_07
healpix/special/other/27/2709/coadd-special-other-2709.log_06
healpix/special/other/27/2709/input.lst-coadd-special-other-2709_09

Can these be removed from the public copy? Note that I did not find any extraneous files in the rv_output directories.

@segasai
Copy link

segasai commented Jan 16, 2025

Thank you!
Definitely, those do not need to be public (and probably should have been deleted by Carlos)

@weaverba137
Copy link
Member

In addition, I've found some empty directories, such as sp_output/230211/healpix/main/backup/368/36844. Can those be removed as well?

@segasai
Copy link

segasai commented Jan 17, 2025

Yes, those should be removed (or I can remove them on my side). In fact the counterpart to those are the rv_output files that are essentially empty (one dummy fits extension file) rv_output/240520/healpix/main/backup/368/36844/rvtab_coadd-main-backup-36844.fits.
Last time I did remove those.
I can do this cleaning myself if that's okay.

@weaverba137
Copy link
Member

This is a bit more complicated now because I've already added checksum files to the copy. It would be easier if you could confirm: if a healpixel is empty in sp_output it should also be removed from rv_output?

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

segasai commented Jan 17, 2025

Sorry.
I double checked now and indeed all 432 empty folders in sp_output (just in case I put them here /pscratch/sd/k/koposov/empty.list) have a corresponding rv_output folder that should be deleted as well.
If useful This is how I checked

for a in `cat  /pscratch/sd/k/koposov/empty.list| sed 's/sp_output\/230211/rv_output\/240520/'` ; do ls -lt $a/rvt*fits ; done |less
for a in `cat  /pscratch/sd/k/koposov/empty.list| sed 's/sp_output\/230211/rv_output\/240521/'` ; do ls -lt $a/rvt*fits ; done |less

Thank you

@weaverba137
Copy link
Member

Thank you. I think this is now completely ready to go. Please take a look at /global/cfs/cdirs/desi/vac/dr1/mws/iron/v1.0. Note that the original README.md file is retained as README.md.bak until that information can be moved into desidatadocs.

@segasai
Copy link

segasai commented Jan 17, 2025

Great! Thank you. I will take a look.
I am pretty confident that I will update the README at some point as well.

@weaverba137
Copy link
Member

At this point, please update the readme via desidatadocs, not a file on cfs.

@dylanagreen
Copy link
Author

@segasai I have opened a PR and branch mws-dr1 on desidatadocs for you to update the README at: https://github.com/desihub/desidatadocs/pull/100. Please make any updates to the README on that branch, so that they are added to the PR, and request a review when done. I copied the README directly from EDR, but have updated the data access section, so there will probably need to be some major edits to bring it in line with DR1.

@dylanagreen
Copy link
Author

@segasai Can you confirm that the files in the staging location look good and there are no further updates? I have merged in the README, and once you sign off on the files we can move them to the final location and finalize this VAC.

@segasai
Copy link

segasai commented Feb 11, 2025

Yes, I think everything looks fine from my point of view.
Thanks

@dylanagreen
Copy link
Author

Thanks, @weaverba137 now that Sergey signed off, let's move these into the final location and I'll close this issue off, thanks for everyone's work on this!

@weaverba137
Copy link
Member

Done. Close when ready.

@segasai
Copy link

segasai commented Feb 26, 2025

Hi,

I understand that this is not great, but I've found out that I missed creating combined tables for special/bright, special/backup, special/other survey/program combinations.

Because of that I'm wondering if it is possible to update 55 files in the MWS VAC, given here (10 of these are new files with the same structure as others, the rest have been recreated).

New files

/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-special-other.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-special-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-special-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-special-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-special-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-special-other.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-special-other.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-special-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-special-bright.fits

Recreated existing files

/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-sv1-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-main-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-sv3-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-sv1-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-cmx-other.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-sv2-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-main-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-sv2-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-sv2-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-sv3-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-sv1-other.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-main-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-sv3-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-special-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240521/rvpix_exp-sv1-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-main-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-sv2-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-sv3-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-sv3-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-main-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-sv1-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-sv1-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-special-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-sv3-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-sv1-other.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-sv2-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-cmx-other.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-sv1-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-main-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/rv_output/240520/rvpix-sv2-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/mwsall-pix-iron.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-sv3-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-sv2-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-main-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-sv2-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-sv1-dark.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-sv2-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-sv1-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-sv3-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-main-backup.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-sv1-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-main-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-sv3-bright.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-sv1-other.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-cmx-other.fits
/global/cfs/projectdirs/desi/science/mws/redux/dr1/v0.94/sp_output/230211/sppix-special-dark.fits

Nothing else would need changing (other than me providing a PR for desidatadocs with the updated numbers)

Sorry for this.

@weaverba137
Copy link
Member

This is definitely not ideal, but we can fix this. It is not as easy as just updating files though, because the checksum files also need to be updated. Basically this update will involve writing a script; it's not something we can fix immediately.

Please get started on that PR for desidatadocs right away.

@weaverba137 weaverba137 reopened this Feb 27, 2025
@segasai
Copy link

segasai commented Feb 27, 2025

Thank you!
I'll open a docs issue a bit later today.
I can also try to help with the script if helpful/i can understand what needs to be done.

@weaverba137
Copy link
Member

Let me concentrate on the script for file replacement. There are additional subtleties related to the design collaboration account and permission locks that require some expert knowledge.

@weaverba137
Copy link
Member

I moved mws out of the public area back to the staging area and performed the updates. Please take a look at $CFS/desi/vac/dr1/mws/iron/v1.0.

@segasai
Copy link

segasai commented Feb 27, 2025

Thank you. I'll be checking now and report.
In the meantime I've updated the readme in desidatadocs https://github.com/desihub/desidatadocs/pull/131

@segasai
Copy link

segasai commented Feb 27, 2025

I've checked and everything looks good.
Thanks again!

@weaverba137
Copy link
Member

OK, I'll move it back to the public area shortly.

@weaverba137
Copy link
Member

The updated mws files are in place.

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