HiPERCAM / hipercam

Python package for handling HiPERCAM data
4 stars 4 forks source link

Reduce plotting breaks if not all CCDs have apertures #123

Closed martinjohndyer closed 6 months ago

martinjohndyer commented 6 months ago

On behalf of @SGParsons (I assume that's the right user?)

Steps to reproduce:

  File "/usr/local/python-3.11.4/bin/reduce", line 8, in <module>
    sys.exit(reduce())
             ^^^^^^^^
  File "/usr/local/python-3.11.4/lib/python3.11/site-packages/hipercam/scripts/reduce.py", line 724, in reduce
    update_plots(
  File "/usr/local/python-3.11.4/lib/python3.11/site-packages/hipercam/reduction.py", line 993, in update_plots
    res = next(res for (this_cnam, res) in results if this_cnam == cnam)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
StopIteration

The error comes from these lines in the reduction.update_plots:

https://github.com/HiPERCAM/hipercam/blob/0778da76f1c30f84b8fde93a127b27fc802b6fc4/hipercam/reduction.py#L991-L997

We're checking if the CCD name is in rfile.aper (an instance of aperture.MccdAper), but with the way the .ape file is formatted even if you don't put down any apertures in a CCD it still gets an empty CcdAper in that group, so if cnam in rfile.aper is always True. The difference is that the results list will only contain entries for CCDs that have apertures, thanks to the len() check here:

https://github.com/HiPERCAM/hipercam/blob/0778da76f1c30f84b8fde93a127b27fc802b6fc4/hipercam/reduction.py#L1374-L1380

So results will be a list of multiple tuples, e.g. [('1', ...), ('2', ...), ...], but only including the CCDs that have apertures. That means next will fail for CCDs not in that list, since there's nothing to iterate through.

Fixing is simple, on line 991 you could just add in the same check to see if that CCD has any apertures to plot:

if cnam in rfile.aper and len(rfile.aper[cnam]) > 0:

although we don't actually use rfile.aper, so perhaps a neater solution would be replacing line 991 with something like

if cnam in [r[0] for r in results]:

@StuartLittlefair if you can verify this I'm happy to make a simple PR. To be honest I'm just surprised if this is the first time this has come up, so maybe I'm missing something? I guess it's rare people don't have apertures in all 5 CCDs, Steven was using WHT commissioning data which had nothing in CCD1 when he found this.

EDIT: Having said all that, I suppose the correct usage in the case where you don't have apertures in all CCDs is to not plot the missing ones (i.e. not use ccd = 0), which makes the issue moot. But maybe there's a reason you'd want to show the frames even if you're not reducing them? Regardless, it's a simple fix so worth doing I'd say.

StuartLittlefair commented 6 months ago

This is a duplication of the (terribly rushed and not well explained) issue #115. Even if you disable plotting for the CCD that has no apertures, reduce still fails to work.

I think the solution

if cnam in rfile.aper and len(rfile.aper[cnam]) > 0:

is the correct one. Certainly we don't want to change the format of the .ape file and I'm concerned that using the contents of the results list may be fragile in the case of failed reductions, or changes to the code elsewhere.