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

final HDU update #144

Merged
merged 4 commits into from Sep 21, 2018
Merged

final HDU update #144

merged 4 commits into from Sep 21, 2018

Conversation

forero
Copy link
Member

@forero forero commented Sep 21, 2018

This PR solves #96. It finally implements all the changes in the HDUs to follow this datamodel
https://desidatamodel.readthedocs.io/en/mockobserving/DESI_TARGET/fiberassign/tile-TILEID-FIELDNUM.html

There are only two fields missing in HDU TARGETS: PSFDEPTH_W1 and PSFDEPTH_W1 that are not included in the latest target files.

@forero forero requested a review from sbailey September 21, 2018 17:09
@moustakas
Copy link
Member

moustakas commented Sep 21, 2018

There are only two fields missing in HDU TARGETS: PSFDEPTH_W1 and PSFDEPTH_W1 that are not included in the latest target files.

These were dropped in desihub/desitarget#368. A more recent select_mock_targets targets file should look (hopefully!) identical to a "real" (DR7-era) targets catalog.

@forero
Copy link
Member Author

forero commented Sep 21, 2018

desihub/desitarget#368 is about mocks, but those two fields are also missing in the latest targets-dr7.1-PR372.fits

@moustakas
Copy link
Member

desihub/desitarget#368 is about mocks, but those two fields are also missing in the latest targets-dr7.1-PR372.fits

Apologies for not being clearer. They're missing from targets-dr7.1-PR372.fits because they only ever existed as output files from select_mock_targets. I added them because I was playing around with a WISE depth model, which we don't track in the data.

desihub/desitarget#368 brought the mock and data targets catalogs into sync, so PSFDEPTH_[W1,W2] columns should be removed from the fiberassign data model. In fact, HDU4 should be defined by https://desidatamodel.readthedocs.io/en/mockobserving/DESI_TARGET/targets.html (which is what I am striving to do in select_mock_targets).

@forero
Copy link
Member Author

forero commented Sep 21, 2018

OK. Now I get it. Then this PR is ready to merge.

bin/fiberassign Outdated
'SHAPEDEV_R', 'SHAPEDEV_E1', 'SHAPEDEV_E2',
'SHAPEEXP_R', 'SHAPEEXP_E1', 'SHAPEEXP_E2',
'FIBERFLUX_G', 'FIBERFLUX_R', 'FIBERFLUX_Z',
'FIBERTOTFLUX_G', 'FIBERTOTFLUX_R', 'FIBERTOTFLUX_Z', 'HPXPIXEL']
Copy link
Contributor

Choose a reason for hiding this comment

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

My intension for the TARGETS HDU was that it would be a literal copy of the MTL rows, reordered to match the order of the fiber assignments, without any hardcoded list of expected columns. That way if the MTL adds or removed columns in the future, they automatically get propagated without also requiring updates to fiberassign. i.e. let the input MTL itself define which columns should be propagated (all of them), and then we'll update the datamodel documentation to match.

Is there some technical reason why you needed to have an explicit list of columns, or could you just propagate all of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did that to avoid duplicates in the columns (i.e. DESI_TARGET and DESI_TARGET_1).

Related question: for the 5000 fibers some targets will come from mtl but others will come from sky. Should I then only include the targets that come from mtl? Right now after the join the targets that come from sky are masked.

Copy link
Member Author

Choose a reason for hiding this comment

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

The code has changed in this part. I am passing now all columns while still masking the targets that didn't come from mtl.

@sbailey
Copy link
Contributor

sbailey commented Sep 21, 2018

Looks good; thanks. Merging now.

Heads up: Julien has convinced me that propagating the smaller FIBERASSIGN table separately from the full TARGETS table is just a pain that is trying to solve a problem that doesn't exist, and that we should just put everything in FIBERASSIGN and move on. I need to confirm with Klaus and Steve Kent that that won't cause some problem for ICS / PlateMaker before we do that. No action needed from us yet...

@sbailey sbailey merged commit c1b9a42 into master Sep 21, 2018
@sbailey sbailey deleted the newcolumns branch September 21, 2018 23:30
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

3 participants