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

Night_qa: introduce new ctedet row-by-row diagnosis plot #2312

Merged
merged 7 commits into from
Aug 7, 2024
Merged

Conversation

araichoor
Copy link
Contributor

This PR introduces a new plot to diagnose possible cte occurences.

The plot was suggested by @schlafly here: https://desisurvey.slack.com/archives/C01HNN87Y7J/p1722543734755599?thread_ts=1722538227.361809&cid=C01HNN87Y7J
Copying from there: "load the 1 s flat, do a row by row extraction, plot the residuals."

In addition to @schlafly initial "recipe", I made these changes:

  • subtract the median value of the residuals (res_median, reported on the plot);
  • restrict the color range to (-2, 2);
  • add per-amplifier infos:
    • the amplifier name, with its related region on the CCD (using 0-index convention);
    • any existing correction (OFFCOLS and/or CTECOLS): column range is reported, along with a line to visualize it.

The plots come as a new pdf file (ctedetrowbyrow-NIGHT.pdf), with one page per petal, three plots (for each camera) per page.
Here is one example for a petal for 20230822:
Screenshot 2024-08-07 at 11 50 29 AM

And here is another example for 20240730, illustrating the "newly" cte on r1:
Screenshot 2024-08-07 at 11 56 06 AM

Here are the nightqa-NIGHT.html pages for those two examples:
https://data.desi.lbl.gov/desi/users/raichoor/nightqa_dev/nightqa_v27/aug07/nightqa/20230822/nightqa-20230822.html
https://data.desi.lbl.gov/desi/users/raichoor/nightqa_dev/nightqa_v27/aug07/nightqa/20240730/nightqa-20240730.html

I think that the code now reads twice the 1s flat preproc images (and I also had to switch from a simple reading with fitsio to a more complete/time-consuming reading with desispec.io.read_image()), that may take an extra minute? but I guess it s ok.
The new plot also calls correct_cte.get_rowbyrow_image_model(), which may take 1 extra minute.

Any suggestion is welcome!

@araichoor
Copy link
Contributor Author

I notice that the "documentation" check didn t pass...

@schlafly
Copy link
Contributor

schlafly commented Aug 7, 2024

sphinx is being strict about the indentation under mydicts; I think if you align those lines it will be quiet.

@araichoor
Copy link
Contributor Author

thanks! that did fix it.

@akremin
Copy link
Member

akremin commented Aug 7, 2024

Thanks Anand. I'd like @schlafly to comment on your choices of restricting the color range and removing the median. The code looks fine to me (I admittedly didn't attempt to understand every line).
How much testing has been done for this? Did you just run it on one recent night or on a few random nights to make sure it's robust? And will this gracefully skip early nights that don't have a 1s flat, or will it crash with an error?

@araichoor
Copy link
Contributor Author

@akremin : thanks for the feedback.
I just tested on two nights...

about missing 1s flat: the approach is the same as for the previous ctedet diagnosis, ie desi_night_qa won t try to generate the pdf if no image is found:

desispec/bin/desi_night_qa

Lines 170 to 173 in 094c3be

if "ctedet" in steps_tbd:
if ctedet_expid is not None:
create_ctedet_pdf(outfns["ctedet"], args.night, args.prod, ctedet_expid, args.nproc)
create_ctedet_rowbyrow_pdf(outfns["ctedetrowbyrow"], args.night, args.prod, ctedet_expid, args.nproc)

one other thing I could think of: if some camera / petal are missing; I ve not tested that; here, it should have the same behavior as for the dark diagnosis (I adapted that piece of code), i.e. just create an empty figure, so I think it should be robust.
it s here in the code:

if mydict is not None:

if you can provide me an example of a 1s flat exposure with missing camera / petal, I can surely run a test (I ll try to look for such an exposure, but you re likely be faster than me, here).

about the choices of restricting the color range and removing the median: sorry, I should have mentioned, I ve already iterated with @schlafly on that.

@akremin
Copy link
Member

akremin commented Aug 7, 2024

20240723 had petal 7 missing from its configuration. Would that test your concern? It doesn't have a BADCAMWORD set in the pipeline exposure_table because the camera wasn't in the instance. If you need a night where we flagged a camera as bad I can dig further. Those are quite rare for calibrations.

@araichoor
Copy link
Contributor Author

thanks that should be perfect for testing (I see /global/cfs/cdirs/desi/spectro/redux/daily/preproc/20240723/00245086/ has no petal=7 data).

@schlafly
Copy link
Contributor

schlafly commented Aug 7, 2024

Anand and I chatted separately re color scales; this looks good to me.

@araichoor
Copy link
Contributor Author

as usual, @akremin, thanks for raising those points.
I actually had a small bug in handling that case; and I also fixed minor things in desi_night_qa.

now the code runs well if one petal is missing:
Screenshot 2024-08-07 at 3 23 11 PM

full night qa files for 20240723 here:
https://data.desi.lbl.gov/desi/users/raichoor/nightqa_dev/nightqa_v27/aug07/nightqa/20240723/ctedetrowbyrow-20240723.pdf

@akremin
Copy link
Member

akremin commented Aug 7, 2024

Thank you, Anand. The plots in that last post look great. Are you happy with this PR now or are you still planning to make changes?

@araichoor
Copy link
Contributor Author

as far as I m concerned, I m happy with the current PR version.
don t know if @schlafly would have any further suggestions?

@schlafly
Copy link
Contributor

schlafly commented Aug 7, 2024 via email

@akremin akremin merged commit 9a4ece1 into main Aug 7, 2024
26 checks passed
@akremin akremin deleted the nightqa_v27 branch August 7, 2024 23:12
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.

3 participants