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

Qlstatus #659

Merged
merged 10 commits into from Jul 17, 2018
Merged

Qlstatus #659

merged 10 commits into from Jul 17, 2018

Conversation

rkehoe
Copy link
Contributor

@rkehoe rkehoe commented Jul 13, 2018

This PR is a small initial installment on cleaning up brittle or old code in QL that relates to the processing and data model of QA metrics. It includes changes to:

  • fix a problem with setting the QA summary STATUS based on configured specifications (i.e. NORMAL_RANGEs or WARN_RANGES). This fix has already been propagated into master
  • extraction of chosen field of scalar metric names from QL configuration, rather than hard-coded within Python. This facilitates one point of indicating the scalar metrics, and allows a more robust propagation of this choice throughout the pipeline -- this will continue in further branches.
  • removal of hardcodied, and outdated run-time parameters when such parameters are not given as QA arguments. These hardcoded PARAMs blocks have gotten out-of-step with code development, are brittle, and should be replaced with error handling to ensure QL is configured properly. An initial exception is thrown in one of these instances, and further branches will continue to remove hard-coded default run-time parameters.

This PR presents almost no change to external behavior, data model or processing to the user, such as QLF. However, to ensure scalar metrics and their specifications have consistent naming, four specifications required slight modifications to their naming:

  • BIAS_NORMAL_RANGE --> BIAS_AMP_NORMAL_RANGE
  • NOISE_NORMAL_RANGE --> NOISE_AMP_NORMAL_RANGE
  • DELTAMAG_NORMAL_RANGE --> DELTAMAG_TGT_NORMAL_RANGE
  • FIDSNR_NORMAL_RANGE --> FIDSNR_TGT_NORMAL_RANGE

The same revision for the WARN_RANGE specifications is also implemented in the qlconfig_*.yaml files.

@Srheft please test this PR to ensure it produces NORMAL values of QA STATUS where appropriate and otherwise works as the current master.

@felipelm, this change requires a modest edit in QLF if it is using the specifications from the
configuration file or as stored in the output JSON files. Please take a look at this PR and implement a parallel PR implementing necessary QLF changes. The intent would be to merge the two PRs when QLF is in-step with this PR.

@felipelm
Copy link
Member

@rkehoe did the changes at desihub/qlf#177

@felipelm
Copy link
Member

Please let me know when this is merged so I can point to the correct commit on master

@Srheft Srheft merged commit 562f901 into master Jul 17, 2018
@rkehoe rkehoe deleted the qlstatus branch November 13, 2018 21:03
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.

None yet

4 participants