Fix parsing GRACES sequences and calendar availability #419
Conversation
|
The last changes to greedymax are dependent on the following lucupy change |
There was a problem hiding this comment.
Overall, the logic here is rather confusing and I'm not sure that the code is maintainable to someone else. Time permitting, let's see if we can simplify this (overall and by possibly extracting some of the functionality to private methods) or at least add more comments about what we are doing in the steps.
Approved now since we're on a time crunch, but I'll create a low priority Jira task to come back and revisit this when we have a chance or someone needs a cognitive shift.
| if instrument in OcsProgramProvider.FPU_FOR_INSTRUMENT: | ||
| if OcsProgramProvider.FPU_FOR_INSTRUMENT[instrument] in step: |
There was a problem hiding this comment.
It seems like these if statements should be combined?
if instrument in OcsProgramProvider.FPU_FOR_INSTRUMENT and OcsProgramProvider.FPU_FOR_INSTRUMENT[instrument] in step:
fpu = step[OcsProgramProvider.FPU_FOR_INSTRUMENT[instrument]]Not really a big deal, though.
There was a problem hiding this comment.
We could also probably simplify this down for instrument logic and fpu logic by using the dict method get(key), which returns None if key is not defined and the value of key otherwise. Just an FYI for future times this comes up.
| # fpu = '1-fiber' | ||
| # elif 'HeIIC' in filt: | ||
| # fpu = '2-fiber' | ||
| if instrument == 'GRACES': |
There was a problem hiding this comment.
Repeated strings like this littered throughout the code are magic values: we should probably just define a str at the class level, graces = 'GRACES', and then use that in these computations instead of repeatedly referencing 'GRACES' directly, but I'll make that change if we have time.
| # Don't consider acq steps | ||
| if step[OcsProgramProvider._AtomKeys.OBS_CLASS].upper() not in [ObservationClass.ACQ.name, | ||
| ObservationClass.ACQCAL.name]: |
There was a problem hiding this comment.
It might be easier to just do:
if step[OcsProgramProvider._AtomKeys.OBS_CLASS].upper() in [ObservationClass.ACQ.name, ObservationClass.ACQCAL.name]:
continueto avoid heavy nested if statements. Also, since we've reused [ObservationClass.ACQ.name, ObservationClass.ACQCAL.name] a couple times now, it would be better practice for us to add this at the top of the class as a static member like:
acquisitions = frozenset([ObservationClass.ACQ.name, ObservationClass.ACQCAL.name])or make a private method:
def _check_step_is_acquisition(step: dict) -> bool:
return step[OcsProgramProvider._AtomKeys.OBS_CLASS].upper() in [ObservationClass.ACQ.name, ObservationClass.ACQCAL.name]| observe_class = step[OcsProgramProvider._AtomKeys.OBS_CLASS] | ||
| step_time = step[OcsProgramProvider._AtomKeys.TOTAL_TIME] / 1000 | ||
|
|
||
| # Any wavelength/filter change is a new atom | ||
| if step_id == 0 or (step_id > 0 and wavelengths[step_id] != wavelengths[step_id - 1]): | ||
| next_atom = True | ||
| # logger.info('Atom for wavelength change') | ||
| # print(f'\t\t\t Atom for wavelength change') | ||
|
|
||
| # A change in exposure time or coadds is a new atom for science exposures | ||
| # print(f'\t\t\t {step[OcsProgramProvider._AtomKeys.OBSERVE_TYPE].upper()}') | ||
| if step[OcsProgramProvider._AtomKeys.OBSERVE_TYPE].upper() not in OcsProgramProvider._OBSERVE_TYPES: | ||
| if (prev >= 0 and observe_class.upper() == ObservationClass.SCIENCE.name and step_id > 0 and | ||
| (exposure_times[step_id] != exposure_times[prev] or coadds[step_id] != coadds[prev])): | ||
| if observe_class.upper() not in [ObservationClass.ACQ.name, ObservationClass.ACQCAL.name]: |
There was a problem hiding this comment.
Here's another place where having a function to check this would avoid repeated code.
|
|
||
| # A change in exposure time or coadds is a new atom for science exposures | ||
| # print(f'\t\t\t {step[OcsProgramProvider._AtomKeys.OBSERVE_TYPE].upper()}') | ||
| if step[OcsProgramProvider._AtomKeys.OBSERVE_TYPE].upper() not in OcsProgramProvider._OBSERVE_TYPES: |
There was a problem hiding this comment.
Something like this would work for the acquisitions, too.
| if (prev >= 0 and observe_class.upper() == ObservationClass.SCIENCE.name and step_use > 0 and | ||
| (exposure_times[step_use] != exposure_times[prev] or coadds[step_use] != coadds[prev])): |
There was a problem hiding this comment.
The logic here is rather confusing: I think it merits a comment for maintainability.
GRACES sequences are non-standard so the acquisition steps have to be ignored.
Since GRACES is accessed via GMOS-N it does not appear in a Port column in the telescope calendar. We check the GRACES availability if GMOS-N is also available. (Note, use of the port columns and the instrument columns is redundant. In the future we could simplify this by just using the instrument columns for availability and adding the port as part of the configuration in case it is needed, eg. for AGS. This is low priority).