desihub / desispec

DESI spectral pipeline
BSD 3-Clause "New" or "Revised" License
36 stars 23 forks source link

7 of 7: Updates to Merging process: Regrouping of QA outputs by Task #584

Closed Srheft closed 6 years ago

Srheft commented 6 years ago

This ticket will be :

NOTE that: These changes would also necessitate that the schema for the offline QAs also migrate from PipelineSteps grouping to Task grouping. This would help ensure compatibility between the online and offline data structures, which in turn is necessary to avoid complicating QLF display and database support.

profxj commented 6 years ago

I think I can support this change to the schema but should like it better detailed before we head there. Here is how things are currently organized in an offline file:

      FLUXCALIB:
        METRICS:
          MAX_ZP_OFF: [0.0, 89]
          NSTARS_FIBER: 10
          RMS_ZP: 0.0
          ZP: 18.05374055054051
        PARAMS: {MAX_ZP_OFF: 0.2, ZP_WAVE: 8250.0}
      SKYSUB:
        METRICS:
          MED_RESID: 1.3428230285644531
          NBAD_PCHI: 32
          NREJ: 0
          NSKY_FIB: 40
          RESID_PER: [-63.823612213134766, 87.43571166992143]
          RESID_RMS: []
          SKY_FIBERID: [4, 7, 14, 17, 30, 36, 98, 104, 139, 140, 160, 172, 182, 
215,
            227, 232, 241, 249, 250, 282, 302, 316, 320, 328, 379, 382, 396, 403
,
            421, 425, 428, 430, 435, 461, 463, 484, 485, 487, 491, 493]
          SKY_WARN: []
        PARAMS: {BIN_SZ: 0.1, PCHI_RESID: 0.05, PER_RESID: 95.0}

How would this evolve in the new schema?

rkehoe commented 6 years ago

Thanks @profxj. Some of these metrics you mention are unfamiliar to me, but some I know. By the conventions that we have in QuickLook, the following items land in different places:

On this last point, the grouping of metrics can be done when taking dictionaries from each QA and merging them. For QL, which includes a monitoring function, these 'tasks' are named accordingly 'CHECK_*'. In the offline, the monitoring function may not be as prominent, but validating performance of algorithms would be. For instance, RESID_PER or RESID_RMS seem more geared toward making sure that sky subtraction is working properly, rather than whether the AMPs are malfunctioning. So perhaps offline has different 'Tasks' like 'VALIDATE SKY', or something like that. Of course, offline can use the same task names QL uses if appropriate. But the comments we received were in part directed at making this grouping/naming more transparent or obvious to the observers who are not developers of the code.

profxj commented 6 years ago

Ok, I think I follow but if offline QA is not task based (it isn't) how do we envision 'migrating from Pipeline steps to Tasks' as @Srheft describes? Maybe it is as simple as packaging, e.g. the fluxcalib offline QA is now described as "AssessFluxCalib".

Is this what you have in mind?

Srheft commented 6 years ago

@profxj: Yes! "packaging" is a good way to explain what it. It's just trying to not confuse the observer by algorithmic names and only grouping the QAs by the task that each group accomplishes. This would only happen in the merged JSON [Kyle would be a good ref for more discussion on this] to make the JSONs more human readable. In other words, the observer cares whether: 1- HDUs are OK 2- CCDs are OK 3- FIBERS are OK 4- SPECTRA are OK

So how ever number of QAs we have, we would group them in these 4 categories in the output JSON. Still open to suggestions/comments.

rkehoe commented 6 years ago

@profxj et al, I think one way to think about this is to provide groupings that have meaning to non-developers, and provide an indication of function. So observers don't want to see step 'Preproc' which doesn't mean much in terms of the metrics, but want to know the first order mission of the quantities they are examining. So if a metric is designed to test flux-calibration integrity, a tag like you mentioned makes sense. I would @ Kyle Dawson here, but can't find him on GitHub?

profxj commented 6 years ago

Ok, but at the end of the day, I doubt anyone will actually look at these files. What really matters is what makes it into the tools we use to view the outputs.

Srheft commented 6 years ago

Agreed. In the end looking into jsons might only come handy for expert observer who digs in more than usual [especially during the commissioning]. This ticket basically came from a discussion while looking at a rather large merged json and trying to make sense of it.

ricardogando commented 6 years ago

A related note, there was a discussion on desi-ql-qlf-int@desi.lbl.gov (not sure if @profxj is on it) about nested lists on dictionaries. https://desi.lbl.gov/trac/tracmailman/browser/private/desi-ql-qlf-int/2018-April/000070.html From @felipeaoli email

What puzzled me is that you have a nested list-dictionary 
( *dict{ list( dict{list( dict{... } ) }* )     }) 
and not just simply nested dictionary. 
For instance, to access a metric, one should use something like:
    My_json_file['NIGHTS'][0]['EXPOSURES'][0]['CAMERAS'][0]['
PIPELINE_STEPS'][1]['METRICS']['XWSIGMA'][0]

This affects the JSON "hashness". Can we make sure that this does not happen in any of the JSONs being produced? Also, do we have a JSON example from @Srheft and @profxj for comparison?

profxj commented 6 years ago

Here is a snippet from the merged JSON file for offline:

{
 "20191216": {
  "332": {
   "flavor": "flat",
   "meta": {
    "DATE-OBS": "2019-12-16T22:00:00.000",
    "NIGHT": "20191216",
    "EXPTIME": 10.0,
    "FLAVOR": "flat"
   },
   "z2": {
    "FIBERFLAT": {
     "METRICS": {
      "CHI2PDF": 1.002207461737427,
      "MAX_MEANSPEC": 126531.9296875,
      "MAX_MEAN_OFF": [
       0.0023685171206955236,
       327
      ],
      "MAX_OFF": 0.08065810015496755,
      "MAX_RMS": [
       0.0058999921311901245,
       127
      ],
      "MAX_SCALE_OFF": [
       0.0023598724078164324,
       14
      ],
      "N_MASK": 0
     },

It is nested: [night][exposure][camera][QAtype]

This structure makes it easy to merge individual QA files for exposures, nights, camera, etc. You'll have to educate me about its 'hashness'. ;)

Srheft commented 6 years ago

@felipeaoli I might have missed the 'hashness' notion if it has been part of any email. If you could email the problem you faced [whether it is a calling sequence issue or something else], that would be great. Here is a snippet of the most recent "online" merged JSON:

{ "NIGHTS": [ { "EXPOSURES": [ { "CAMERAS": [ { "CAMERA": "z2", "GENERAL_INFO": { "AIRMASS": 1.0, "B_PEAKS": [ 3914.4, 5199.3, 5201.8 ], "DEC": [ ] },

                            }
                        ]
                    }
                ],
                "EXPID": 3900,
                "FLAVOR": "science",
                "PROGRAM": "dark"
            }
        ],
        "NIGHT": "20191017"
    }
]

}

felipeaoli commented 6 years ago

Hi all,

My original question was wondering if there is a reason for the online json being made by nested single valued lists of dictionaries. Please Xavier, notice that this is different from the offline json you've showed here, in which you have only a nested dictionary. The online json places almost every dictionary inside a single element list.

If there is no reason for the current structure of online json (and, of course, I may be missing the reason) it seems to me a good a ideia to keep the data in a nested dictionary.

Finally, if there is a reason for adopt the single valued lists in online json, we here can deal with that. I just raised my question because I need the json structure for our QAs visualization (and a simpler one seems more reasonable).

Best Regards and sorry for the long message,

Felipe.

Em quarta-feira, 9 de maio de 2018, Sarah E notifications@github.com escreveu:

@felipeaoli https://github.com/felipeaoli I might have missed the 'hashness' notion if it has been part of any email. If you could email the problem you faced [whether it is a calling sequence issue or something else], that would be great. Here is a snippet of the most recent "online" merged JSON:

{ "NIGHTS": [ { "EXPOSURES": [ { "CAMERAS": [ { "CAMERA": "z2", "GENERAL_INFO": { "AIRMASS": 1.0, "B_PEAKS": [ 3914.4, 5199.3, 5201.8 ], "DEC": [... ... ] },

                        }
                    ]
                }
            ],
            "EXPID": 3900,
            "FLAVOR": "science",
            "PROGRAM": "dark"
        }
    ],
    "NIGHT": "20191017"
}

]

}

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/desihub/desispec/issues/584#issuecomment-387892856, or mute the thread https://github.com/notifications/unsubscribe-auth/AcdlMqmrVehpcOXllbIuM2tVkkrHDQyQks5tw2uegaJpZM4TLyL2 .

Srheft commented 6 years ago

Thank you for your comment @felipeaoli.

This "dict-list-dict" structure is because it has been decided [long before my time] that we may need to at some point merge the dictionaries of a number of nights together. As I mentioned in calls, we could abandon this merging nights/exposures feature and that would peel off two layers in the calling sequence. So far, we have not used this merging night/exposure option so that's why you see a single element list of one night and list of one exposure and list of one camera. Depending on how bothersome it has been for QLF, we could think about removing those features.

@sbailey, @profxj, @ricardogando , All: thoughts? votes?

ricardogando commented 6 years ago

@Srheft I think what @profxj just showed is the way to go. Dictionaries only, so you can directly access the keys, not having to guess an index on the list. I am not sure I follow how the lists would ease the merge. (note on the dictionary shared by @profxj, it does not follow the key:value on all levels, but I do not have a strong opinion about that - and it was also decided otherwise in the recent past -
and I understand that the way it is presented right now can be more effective when accessing nights and exposures, once the data model is set on stone).

sbailey commented 6 years ago

@Srheft could you re-post your recent QL output snippet example wrapped in pre-formatted text triple-backticks? The github markdown formatting munged it and I'm having a hard time interpreting it. I think what was intended was:

{
  "NIGHTS": [
    {
      "NIGHT": "20191017"
      "EXPOSURES": [
        {
          "EXPID": 3900,
          "FLAVOR": "science",
          "PROGRAM": "dark"
          "CAMERAS": [
            {
              "CAMERA": "z2",
              "GENERAL_INFO": {
                "AIRMASS": 1.0,
                "B_PEAKS": [3914.4, 5199.3, 5201.8],
                "DEC": [...]
              },  # end GENERAL_INFO dict for camera z2
            }
          ]  # end CAMERAS list
        }
      ], # end EXPOSURES list
    }
  ], # end NIGHTS list
}

(but I'm not sure...) Please expand that down to an example QA metric.

The simpler structure that @profxj described of nested dictionaries [night][exposure][camera][QAtype] also allows merging (or splitting) across multiple levels (which was also the original motivation for that scheme), but we'd need to figure out how to include metadata information like FLAVOR / PROGRAM / AIRMASS that belong to an exposure but aren't cameras.

I'm still having a hard time understanding the details of the QL format in order to map that on to the simpler nested structure, so I'd like to make sure I got the hierarchy right first. It would be helpful to get a tangible example mapped into both schemes to highlight whether anything really doesn't fit.

Srheft commented 6 years ago

@sbailey: the structure appears like the one I posted earlier and continues to look like that when I just tried. Your idea of what it is correct. have sent the latest json to @profxj and @felipeaoli [and others] as well.