QL config update and fibermap conform #699

Srheft · 2018-10-11T18:33:49Z

This PR:

updates arc configuration file
migrates to science config for all three science programs [dark,gray,bright]
moderate edits to XWSigma and SNR, calculation algorithm and few other QA calculations

@sbailey please see ticket #698.
Keeping with our general rule, this PR will be tested in QLF [with new format data] before merge.

…d fibermaps

…phase II

Srheft · 2018-10-15T17:32:10Z

@felipelm since I don't have writing privileges there please copy the new redwood data set [night 20200515] that QL/F have to work with, from this address on NERSC: /project/projectdirs/desi/datachallenge/redwood/20200515 onto desi-1.

This PR only has two changes for QLF:

Exactly where it exists in the mapping for the Science exposures, XWSigma should be added to CHECK_CCD step for arc exposures processing as well.
instead of having three configuration files for science exposure processing (they were called qlconfig_darksurvey.yaml, qlconfig_brightsurvey.yaml, qlconfig_graysurvey.yaml), there is now only one configuration file called qlconfig_science.yaml. QLF should just make sure to call this config file for all science processing regardless of the dark/gray/bright programs. The internal changes to this file should not affect QLF. The merged json file has the same structure and same status key names.

let me know if it would be helpful to create a PR for the changes on the qlf side. Thanks.

felipelm · 2018-10-16T13:03:29Z

@Srheft you have the same permissions as I because we use the same quicklook user.

Ok, I'll make a PR on qlf with these changes and reference here.

Srheft · 2018-10-16T13:05:43Z

@felipelm last time I tried to copy data on /data directory it said permission denied. Will you copy the redwood data or I should go ahead and do that?

felipelm · 2018-10-16T13:10:40Z

@Srheft I think @sbailey protected the exposures directory to avoid changing or deleting the files.

I also don't have the permissions to change the raw exposures directory.

Srheft · 2018-10-16T13:17:27Z

Ok Thanks for letting me know, @felipelm.
This week is the Barcelona meeting week so hopefully at some point @sbailey find some time to copy the data there. But this PR should be working with 20191001 too so please let me know if the two changes I mentioned above require me to create a PR for qlf.

felipelm · 2018-10-16T13:30:38Z

@Srheft

Got errors running 20191001 exposure 00003578 camera b0 (log is upside down first line is the last and so on):

2018-10-16 13:19:24,153 QuickLook INFO : Running Calc_XWSigma
UnboundLocalError: local variable 'refval' referenced before assignment
    reflist=isinstance(refval,(np.ndarray,collections.Sequence))
  File "/app/desispec/py/desispec/quicklook/qas.py", line 200, in __call__
    res=qa(inp,**qargs)
  File "/app/desispec/py/desispec/quicklook/quicklook.py", line 302, in runpipeline
Traceback (most recent call last):
2018-10-16 13:19:24,126 QuickLook WARNING : Failed to run QA Count_Pixels. Got Exception local variable 'refval' referenced before assignment
2018-10-16 13:19:24,126 Count_Pixels WARNING : No reference given. Update the configuration file to include reference value for QA: Count_Pixels
2018-10-16 13:19:24,053 QuickLook INFO : Running Count_Pixels
UnboundLocalError: local variable 'refval' referenced before assignment
    reflist=isinstance(refval,(np.ndarray,collections.Sequence))
  File "/app/desispec/py/desispec/quicklook/qas.py", line 200, in __call__
    res=qa(inp,**qargs)
  File "/app/desispec/py/desispec/quicklook/quicklook.py", line 302, in runpipeline
Traceback (most recent call last):
2018-10-16 13:19:23,967 QuickLook WARNING : Failed to run QA Get_RMS. Got Exception local variable 'refval' referenced before assignment
2018-10-16 13:19:23,967 Get_RMS WARNING : No reference given. Update the configuration file to include reference value for QA: Get_RMS
2018-10-16 13:19:23,450 QuickLook INFO : Running Get_RMS
UnboundLocalError: local variable 'refval' referenced before assignment
    reflist=isinstance(refval,(np.ndarray,collections.Sequence))
  File "/app/desispec/py/desispec/quicklook/qas.py", line 200, in __call__
    res=qa(inp,**qargs)
  File "/app/desispec/py/desispec/quicklook/quicklook.py", line 302, in runpipeline
Traceback (most recent call last):
2018-10-16 13:19:23,394 QuickLook WARNING : Failed to run QA Bias_From_Overscan. Got Exception local variable 'refval' referenced before assignment
2018-10-16 13:19:23,393 Bias_From_Overscan WARNING : No reference given. Update the configuration file to include reference value for QA: Bias_From_Overscan
...

Srheft · 2018-10-16T13:33:21Z

Yeah this is one change I had to make because of the redwood data. I'll see how can i make it to work for both. Lack of key 'PROGRAM' made me do some changes. Could you try an arc and make the change I mentioned to arc processing?

felipelm · 2018-10-16T13:35:07Z

Arc ran fine but arc XWsigma plot didn't work so I'm trying to debug it now.

Srheft · 2018-10-16T13:36:19Z

Yeah this PR has added xwsigma in arc. Thanks.

felipelm · 2018-10-16T13:47:01Z

@Srheft seems like GENERAL_INFO ra and dec are null values and we expect an array size 500 for each.

felipelm · 2018-10-16T13:48:08Z

Running 20191001 exposure 00003571 camera b0

Srheft · 2018-10-16T13:48:50Z

calibration frames [flat and arc] are not pointed at any set of astronomical objects so the RA DEC arrays are empty. one would only look for them in a science exposure.

felipelm · 2018-10-16T14:42:21Z

@felipeaoli care to give your input on this plot?

Srheft · 2018-10-16T14:57:28Z

you mean the xwsigma plot for arc? the xwsigma plot for any flavor is created here: xsigma plot
This is an example of arc 00000001 camera b0 - the empty panels if exist, will be dealt with

felipeaoli · 2018-10-16T15:00:43Z

Thanks @Srheft and @felipelm . I'm doing a large revision on the plots and I'll add this one.

felipeaoli · 2018-10-19T14:44:08Z

@Srheft we also use reference values in color maps for some cases.

Srheft · 2018-10-19T14:50:39Z

@felipeaoli oh not only I did not know that, I also don't know what color map in this context :)

update: upon remembering bokeh graphics, I just got what you meant :)

@felipelm is everything ok with the science exposure runs now?

felipelm · 2018-10-19T15:50:23Z

@Srheft just finish testing and everything seems ok now.

Srheft · 2018-10-19T15:52:19Z

@felipelm et al thank you very much! I'll proceed with the merge after @rkehoe gave his final remarks.

Srheft · 2018-10-19T17:07:40Z

@felipeaoli did you merge this PR?

Srheft · 2018-10-19T17:08:33Z

@felipelm could you let me know why this merge happened?

felipeaoli · 2018-10-19T17:09:04Z

sorry, I was answering another PR and made a mistake

Srheft · 2018-10-19T17:11:46Z

@felipeaoli I don't see this being reverted. I still see this branch merged.

felipelm · 2018-10-19T17:16:31Z

@felipeaoli @Srheft you have to merge the other PR to revert the changes on master and open a new one with this changes

Srheft · 2018-10-19T17:18:15Z

@felipelm i don't know what is going on here.

felipelm · 2018-10-19T17:24:39Z

@Srheft

@felipeaoli accidentally merged this pull request to master and opened a new one to revert this merge #701

To undo the merge you will have to:

Merge Revert "QL config update and fibermap conform" #701 into master
Open a new pull request using the branch qlconform_NewFibMap_XWSigUpdate

rkehoe · 2018-10-19T17:46:22Z

@felielm and @felipeaoli Please do not merge QL branches/PRs. These should only be merged by @Srheft or myself (or @sbailey) because we are aware of any internal reviews, tests or edits we are pursuing. PR #701 unfortunately does not retain the comment history, and I am unclear on the status of master at this point. @sbailey could you please indicate if master has still the merged code, or has been returned to the pre-merge basis?

Please do no further merge until I can finish examining the final edits. Assuming master is in a pre-merge state, we will merge the outstanding PR.

felipeaoli · 2018-10-19T18:02:54Z

Sure @rkehoe . This was exclusively my fault, I accidentally merged it. I'm very sorry

rkehoe · 2018-10-19T19:13:42Z

@felipeaoli I accept your apology, but then why did you create another PR #702? I have to be clear: do not continue to try to 'fix' this situation in desispec.

@Srheft This PR (#699) looks good and accomplishes the tasks of converging science configs, evening out calibration exposure processing chains, and further improving fitting QAs like XWSigma and SNR. There appears to be another PR built to restore this one, so I am further confused as to the status of this code. I have only one suggestion before signing off on this PR as it was originally before the accidental merge: I think you can go ahead and remove the now obsolete darksurvey, brightsurvey and _graysurvey_configs. We just have science now. I have some other questions/suggestions, but aside from removing the obsolete configs, these should not hold up a final merge of this code.

If master has been restored to pre-merge status, then this PR (#699) should still be able to be remerged into master. We can chat about how to proceed offline. I will contact you this afternoon.

Srheft · 2018-10-19T19:55:44Z

@rkehoe Thanks. given the circumstances, I prefer to withdraw from any further action on this PR until @sbailey could take a look and do what has to be done. I'll apply your suggestions in the next PR (I would add them here if Stephen confirms that it is safe to do so).

sbailey · 2018-10-23T22:30:32Z

@rkehoe once a PR has been merged, it can't be re-merged. What can be done is open a new PR to undo (revert) the accidental merge changes (which @felipeaoli did with PR #701), and then when ready open a new PR to perform the same merge as the original PR when ready (which is what he prepared with PR #702, which correctly references this PR for traceability, but hasn't been merged yet).

@felipelm and @Srheft : please deploy the equivalent of PR #702 and desihub/qlf#253 on http://desi-1.kpno.noao.edu:9090 . When that is tested and it all works together we can proceed with merging those two PRs. desi-1 was offline yesterday for cooling system maintenance but it is back today.

felipelm · 2018-10-24T13:29:30Z

@Srheft @sbailey

I've made the deploy of #702 using desihub/qlf#253 on http://desi-1.kpno.noao.edu:9090

Ran arc, flat, and three science (dark, gray, bright) exposures and seems to be working fine. All plots are showing with the changes made using the new science config file and xwsigma is now showing on arcs CHECK_CCDs.

Seems like the production database is not up so http://desi-1.kpno.noao.edu:9000/ backend is offline.

Srheft · 2018-10-24T15:05:23Z

@felipelm great, thank you.

rkehoe · 2018-10-24T17:04:46Z

@sbailey. Thanks for the clarification. I had thought I had seen a different usage, so good to know going forward.

@Srheft Please merge the final PR #702 when any remaining updates tests are done. If you prefer to hold the original updates to #699 for a subsequent PR, that’s okay for me.

Srheft · 2018-10-24T17:25:34Z

@rkehoe sure. Just one last check with Stephen:
@sbailey did you also want to do some high level tests on desi-1 or given that @felipelm observed no problem, we are ok to merge?

sbailey · 2018-10-24T17:59:29Z

@Srheft @felipelm @rkehoe : let's merge #702 and deishub/qlf#253 and then make new tags. I have meetings for the next 2 hours, but if you haven't made the merges by then I can do it. I'll submit tickets for any problems I find on
http://desi-1.kpno.noao.edu:9090/ so that we don't further delay these PRs. Thanks.

@rkehoe from git diff, #699 and #702 have identical code changes and differ only by #702 having more recent changes.rst and _version.py updates. i.e. I don't think there are any "original updates to #699 for a subsequent PR" that are not already included in #702. If you know of something that is missing from #702, please point that out. Thanks.

…revert QL config update and fibermap conform - #699 changes

rkehoe · 2018-10-24T18:46:07Z

@sbailey. There were three potential small updates, one of which was mentioned in my comment 5 days ago (eg. Removing the now obsolete dark/gray/bright Survey configuration in favor of the merged config). Two others I discussed offline w/@Srfheft, one I remember was about choice of arc-lines to look at in the configuration. None of the three were implemented yet into #699 — they were about to be before the accidental merge. They are not necessary for this PR or #702, but we agreed to do them. They can be in a subsequent PR, for sure.

sbailey · 2018-10-24T20:47:32Z

@rkehoe thanks for the info. +1 for including these changes in separate smaller PRs. The code from this PR has been merged via #702 and tagged as desispec/0.25.0 .

Srheft and others added 14 commits October 8, 2018 03:21

basics aside snr

884d92a

xwsigma reintroduced to arc processing

bee9bec

xwsigma amp exception case update

393f034

xwsigma amp exception case update

92bd856

QL now acknowleges that image.meta["PROGRAM"] does not exist in new data

3188979

xwsigma: minor edit

2602ddc

migrating to one science config instead of program-specific configs

7a20e94

update snr params for new fibermaps

ee3c0eb

QL to not complain about the *inf* magnitude for sky fibers in redwoo…

02d38e4

…d fibermaps

base class update

06754ef

minor edit

1ff076c

some snr cleanup

6ee18da

migrating to one science config instead of program-specific configs- …

070d1d3

…phase II

better suggestions for flat processing

c13ca91

edits to flat processing calculation

e1b5f89

felipelm mentioned this pull request Oct 19, 2018

Fix amp plots desihub/qlf#257

Merged

felipeaoli merged commit 2ae6c02 into master Oct 19, 2018

felipeaoli mentioned this pull request Oct 19, 2018

Revert "QL config update and fibermap conform" #701

Merged

felipelm added a commit that referenced this pull request Oct 19, 2018

QL config update and fibermap conform - #699 changes

b3c44d6

felipelm mentioned this pull request Oct 19, 2018

QL config update and fibermap conform - #699 changes #702

Merged

Srheft added a commit that referenced this pull request Oct 24, 2018

Merge pull request #702 from desihub/qlconform_NewFibMap_XWSigUpdate_…

74921f4

…revert QL config update and fibermap conform - #699 changes

rkehoe deleted the qlconform_NewFibMap_XWSigUpdate branch November 13, 2018 21:04

QL config update and fibermap conform #699

QL config update and fibermap conform #699

Conversation

Srheft commented Oct 11, 2018 • edited

Srheft commented Oct 15, 2018

felipelm commented Oct 16, 2018

Srheft commented Oct 16, 2018

felipelm commented Oct 16, 2018

Srheft commented Oct 16, 2018 • edited

felipelm commented Oct 16, 2018 • edited

Srheft commented Oct 16, 2018

felipelm commented Oct 16, 2018

Srheft commented Oct 16, 2018

felipelm commented Oct 16, 2018

felipelm commented Oct 16, 2018

Srheft commented Oct 16, 2018 • edited

felipelm commented Oct 16, 2018

Srheft commented Oct 16, 2018 • edited

felipeaoli commented Oct 16, 2018

felipeaoli commented Oct 19, 2018

Srheft commented Oct 19, 2018 • edited

felipelm commented Oct 19, 2018

Srheft commented Oct 19, 2018

Srheft commented Oct 19, 2018

Srheft commented Oct 19, 2018

felipeaoli commented Oct 19, 2018

Srheft commented Oct 19, 2018

felipelm commented Oct 19, 2018

Srheft commented Oct 19, 2018

felipelm commented Oct 19, 2018 • edited by Srheft

rkehoe commented Oct 19, 2018

felipeaoli commented Oct 19, 2018

rkehoe commented Oct 19, 2018

Srheft commented Oct 19, 2018 • edited

sbailey commented Oct 23, 2018

felipelm commented Oct 24, 2018

Srheft commented Oct 24, 2018

rkehoe commented Oct 24, 2018

Srheft commented Oct 24, 2018

sbailey commented Oct 24, 2018

rkehoe commented Oct 24, 2018

sbailey commented Oct 24, 2018

Srheft commented Oct 11, 2018 •

edited

Srheft commented Oct 16, 2018 •

edited

felipelm commented Oct 16, 2018 •

edited

Srheft commented Oct 16, 2018 •

edited

Srheft commented Oct 16, 2018 •

edited

Srheft commented Oct 19, 2018 •

edited

felipelm commented Oct 19, 2018 •

edited by Srheft

Srheft commented Oct 19, 2018 •

edited