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

updates for release 17.11 #6

Merged
merged 1 commit into from
Dec 8, 2017
Merged

updates for release 17.11 #6

merged 1 commit into from
Dec 8, 2017

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Nov 14, 2017

This PR updates the minitest notebook for changes in the exposures table in desisurvey and surveysim: column capitalization, new columns, new way to add EXPIDs. See desihub/desisurvey#67, desihub/desisurvey#68, desihub/desisurvey#74, and desihub/surveysim#55. It was tested on a set of release candidate tags for the upcoming desimodules 17.11 release, but it should also work on current master.

@weaverba137 you don't need to spend 2 hours and 1500 MPP hours to rerun this this time, but could you check the outputs in /global/cscratch1/sd/sjbailey/desi/dev/end2end and see if those are sufficient for loading the spectro DB with full associations between bricks, healpixels, targets, exposures, spectra, and redshifts? Thanks.

Copy link
Member

@weaverba137 weaverba137 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are my initial comments after having looked at all the output files, but without having actually loaded the database yet.

  1. The truth and target files have changed significantly since the last attempt. That's not a bad thing, because it looks like all vector-valued columns have been expanded into scalar columns. Just making sure that's intentional.
  2. The truth and targets files don't set units on any values, even obvious ones like RA, DEC. This is a separate issue, don't worry about it for this notebook.
  3. The exposures.fits file should have 'EXTNAME' value on its data. This is a separate issue, don't worry about it for this notebook.
  4. I'd really like to get back to using full UTC timestamps instead of MJD. Databases like timestamps. Again, not an issue for this notebook.
  5. Is there any reason the targetid can't be the first column of the zcatalog file? It's just a convention, but primary keys are usually the first column.
  6. The zcatalog.fits roll-up file doesn't contain the healpix id. If we want healpix ID in the database, I'll have to load from the individual zbest files. Looks like NUMOBS is also missing.
  7. If healpix ids are known at the time of fiberassignment, it would be good to include those. It would possibly simplify some joins.

@sbailey
Copy link
Contributor Author

sbailey commented Nov 14, 2017

Thanks for this review of the outputs. These are all good comments. Sorting and responding:

The truth and target files have changed significantly since the last attempt. That's not a bad thing, because it looks like all vector-valued columns have been expanded into scalar columns. Just making sure that's intentional.

Yes, that was intentional to match the updated format of the tractor/sweep files from the imaging surveys. In the future we'll make a similar update to the fibermap files to change vector-valued columns into multiple scalar-valued columns.

Fix now

These are simple enough (I think) I will try to go ahead and fix them now:

The exposures.fits file should have 'EXTNAME' value on its data. This is a separate issue, don't worry about it for this notebook.

Is there any reason the targetid can't be the first column of the zcatalog file? It's just a convention, but primary keys are usually the first column.

The zcatalog.fits roll-up file doesn't contain the healpix id. If we want healpix ID in the database, I'll have to load from the individual zbest files. Looks like NUMOBS is also missing.

Assuming that we can find the time to do this before the end of the week, let's wait for that instead of implementing loading from zbest files.

Longer term

Good suggestions, but I'll punt these to other tickets:

The truth and targets files don't set units on any values, even obvious ones like RA, DEC. This is a separate issue, don't worry about it for this notebook.

desihub/desitarget#237

I'd really like to get back to using full UTC timestamps instead of MJD. Databases like timestamps. Again, not an issue for this notebook.

desihub/desisurvey#76

Converting MJD -> UTC datestamps for DB loading is the short term soluion.

If healpix ids are known at the time of fiberassignment, it would be good to include those. It would possibly simplify some joins.

desihub/desitarget#238

Thanks again for the useful review of the outputs.

@weaverba137
Copy link
Member

See desihub/desisurvey#75 for EXTNAME on exposures.fits.

@sbailey
Copy link
Contributor Author

sbailey commented Nov 20, 2017

Update: from my "fix now" list, @weaverba137 implemented the EXTNAME in exposures.fits via desihub/desisurvey#75. Thanks. I think the second item of the column order is optional (right?), but getting the HPXPIXEL and NUMOBS in the zcatalog file instead of requiring scanning N>>1 zbest and spectra files would still be great to do. i.e. fixing it in desispec desi_zcatalog would probably be easier than working around those missing quanities in downstream DB loading code. But I haven't found the time to do this yet, and review prep is looming. If @weaverba137 or others have time to contribute, please do. Otherwise I might get to it over Thanksgiving weekend.

@weaverba137
Copy link
Member

The column order is in fact optional. It's just a convention. In principle, the column order of the database table can be different from the input file, but that requires additional code and "hand-holding".

@weaverba137
Copy link
Member

What's the status of this branch? Is it ready to merge? I'd potentially like to base a project for the Friday Hack Day at SLAC on the mini notebook, but should I branch off this branch, or branch off master after this is merged, or branch of master as it is now?

@sbailey
Copy link
Contributor Author

sbailey commented Dec 5, 2017

Remaining issues are simple, just need to find the time. I'll try to wrap up by the end of Wednesday. If that's not soon enough, find me in person and we can chat.

@weaverba137
Copy link
Member

Wednesday is fine, but let me know if it slips much past that.

@sbailey
Copy link
Contributor Author

sbailey commented Dec 8, 2017

Merging now; remaining items will catch up in pending PRs and issues (e.g. desihub/desispec#473 for getting TILEID necessary for calculating NUMTILES and NUMEXP for desihub/redrock#24).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants