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

Ql drilldown metrics update #517

Merged
merged 8 commits into from Mar 9, 2018
Merged

Ql drilldown metrics update #517

merged 8 commits into from Mar 9, 2018

Conversation

gdhungana
Copy link
Contributor

This PR

  • updates the keys for the QL QA metrics as defined in the new model
  • removes the amp level granularity of the 1D level QAs
    Note: Few of the new keys are placeholders that preserves the data structure but need to be calculated correctly. That will be addressed in subsequent update. At the moment this will facilitate for the QLF implementation. @rkehoe @Srheft Please review and comment if you see any inconsistencies or other issues.

@gdhungana gdhungana requested a review from rkehoe March 5, 2018 23:24
Copy link
Contributor

@Srheft Srheft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @gdhungana. My review results are below:

  • I did check the QL_drills branch and compared the name change and array re-structures to the wiki list that we agreed upon.

  • I did check the travis test detail to check for warnings. An open issue on desispec regarding failure to run QA with matplotlib 2.1.0 made me curious to check the compatibility of the plotting schemes with that Matplotib version [we were using earlier version]. The deprecated warnings won't show up once ql_snr is merged.

  • I did run this branch with or without --mergeQA to ascertain a smooth/successful run.

  • Aside from the XWsigma and SNR plots failure in this branch [the fix is in ql_snr], 8 plots were created. The AMP related subplots in countbin, skycont, skypeak and integ plots are removed ofcourse.

The integ plot shows up empty. I assume it is because the INTEG has changed to FIBER_MAG and that is empty for now, right?

@rkehoe: you could start your review.

Regards,
srh

@Srheft
Copy link
Contributor

Srheft commented Mar 7, 2018

@gdhungana

Change the FIBER_AMG back to INTEG so that the plot doesn't show empty.

@gdhungana
Copy link
Contributor Author

@Srheft Comments taken into account. Let me know if there are further issues.

@Srheft
Copy link
Contributor

Srheft commented Mar 8, 2018

@gdhungana
Just to confirm:

This [first attachement] is an integ plot that was being created before [i.e., using master] :
image

and this [second attachement] is the integ plot that this branch [QL_drills] creates:

image

@rkehoe
Copy link
Contributor

rkehoe commented Mar 9, 2018

Thanks @gdhungana. This has some useful cleanup, and really help regularize things and ease usage downstream.

Thanks also @Srheft for catching the missing fibermag information. This isn't calculated correctly, but I'd rather preserve the start of the final calculation than disable or delete it. So I'm glad we have something there for the moment. Once we have a fast flux calibration in place, we can complete the fiber magnitude/imaging magnitude comparison.

I think there are some second order naming effects we should consider, but that can come as part of another PR. This one is ready to merge, which I will do now.

@rkehoe rkehoe merged commit e37dbda into master Mar 9, 2018
@weaverba137
Copy link
Member

Is there any reason not to delete this branch?

@Srheft
Copy link
Contributor

Srheft commented Mar 28, 2018 via email

@weaverba137 weaverba137 deleted the QL_drills branch March 28, 2018 17:51
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