Plotting updates from #106 #110

kreczko · 2018-03-14T20:47:35Z

This PR adds all plotting related changes from #106.

kreczko · 2018-03-14T21:11:36Z

@benkrikler These modules definitely needs some more work, I've created an issue regarding the style information (#109).

Since you wrote most of this, could you please have a quick look at @professor-calculus' and @bundocka's work?

I've tidied things up here and there.

benkrikler

Nothing particularly serious, but many places where we could improve.

benkrikler · 2018-03-17T00:10:16Z

cmsl1t/plotting/efficiency.py

        for threshold in all_pileup_effs.iter_all():
            if not isinstance(threshold, int):
                continue
            hist = all_pileup_effs.get_bin_contents(threshold)
-            hist.drawstyle = "EP"
+            hist.drawstyle = "P"


I guess this should be hist.drawstyle = EfficiencyPlot.drawstyle ?

benkrikler · 2018-03-19T08:07:36Z

cmsl1t/plotting/efficiency.py

-            canvas = draw(hists, draw_args={"xtitle": self.offline_title,
-                                            "ytitle": "Efficiency"})
+            draw_args = {"xtitle": self.offline_title, "ytitle": "Efficiency"}
+            # TODO: special case should not be implemented here!


I agree with this comment! I'd be tempted to say we take this out of the PR so we don't let this sort of issue creep into the codebase, and then we work out a more long-term solution. Need finer control over plotting directly from the yaml config file, I suppose.

You mean like # TODO: Remove when we update rootpy to >0.9.2 :)

I will put it in as it will force me to create the issue. Miss the gitlab feature "resolve with issue"

Sorry, I've no objection to TODO comments in the code, I meant let's take out the actual code below this comment that the comment was referring to. That's the sort of issue I want to avoid creeping in, not an issue of having TODO comments in there!

Ah, that makes more sense ;).

However, at the moment we do not have a better solution, do we?

I was thinking we could take it out of the PR, then let Aaron or Alex put it back in on their branch alone, or uncommitted until we come up with a better solution. If we do let it in, we'd want a very specific issue to be opened, I suppose

The issue I had in mind here is to inject (like analyzers & filters in the new model) Styles into plotting routines.
This is something that needs to be done on our side and I am not sure about the exact form yet.
But having this code in master will force us to do something about it (and serves as a good example for the issue).

Let me create the issue and then we can merge this.

Actually, issue already exists :)
#109

benkrikler · 2018-03-19T08:09:38Z

cmsl1t/plotting/rates.py

+
+def get_cumulative_hist(hist):
+    h = hist.clone(hist.name + '_cumul')
+    arr = np.cumsum(_reverse([bin.value for bin in hist]))


Do we already have root_numpy in the requirements for this framework? If so we could make use of it here instead, perhaps? Probably not worth it 2bh.

Not in this PR at least ;).

def cumulative_sum_and_error(hist): ''' Takes a histogram and returns an array of cumulative sums of it with the total first. E.g. histogram entries: [1, 2, 3, 4] histogram errors: [1, 1, 2, 2] Output: [10, 9, 7, 4], [3.16227766, 3, 2.82842712, 2] ''' hist_values = [b.value for b in hist] reversed_cumsum = _reversed_cumulative_sum(hist_values) errors_squared = np.square([b.error for b in hist]) reversed_cumsum_errors = np.sqrt(_reversed_cumulative_sum(errors_squared)) return reversed_cumsum, reversed_cumsum_errors def _reversed_cumulative_sum(values): reversed_values = np.flipud(values) cumsum = np.cumsum(reversed_values) reversed_cumsum = np.flipud(cumsum) return reversed_cumsum

and the previous code becomes

values, errors = cumulative_sum_and_error(hist) h.set_content(values) h.set_error(errors)

Yeah, this looks good to me. Might even be worth putting this function in a utility module since it could be useful in multiple places. Or even submit a PR to rootpy itself for a new Hist1D method?

→ cmsl1t.utils.hist

benkrikler · 2018-03-19T08:13:22Z

cmsl1t/plotting/rates.py

+    bin1 = h.get_bin_content(1)
+    if bin1 != 0:
+        h.GetSumw2()
+        h.Scale(4.0e7 / bin1)


Where does this magic 4.0e7 come from? Is it something that depends on the run conditions, eg, valid for Run-2 but not for another era, etc? Either way, can we assign a variable with some descriptive name to this value first, then use the variable within this calculation?

40MHz pp collision rate, so it's chosen to just scale everything in reference to that I think.

benkrikler · 2018-03-19T08:18:13Z

cmsl1t/plotting/rates.py

+    return h
+
+
+def _reverse(a):


As far as I can see, this function is only used on 1D lists? If so, you could probably just use the built-in reverse method, reversed(), which would simplify the code and make it easier to read.

Well, the code would then be

arr = np.cumsum(list(reversed([bin.value for bin in hist])))

not convinced that this is better.

That's true reversed would return an iterator object, so you'd need the list. Still, I'm not sure wrapping this simple operation in a function is worthwhile. Wouldn't this work:

arr = np.cumsum(np.flipud([bin.value for bin in hist])))

or even

arr = np.cumsum([bin.value for bin in reversed(hist)]))

or

arr = cumulative_sum(hist)

benkrikler · 2018-03-19T09:17:05Z

cmsl1t/plotting/resolution.py

-            # if with_fits:
-            #     fits.append(self.fits.get_bin_contents([pile_up]))
+            if with_fits:
+                fits.append(self.fits.get_bin_contents([pile_up]))


I think this is a left-over from copying this file from the Efficiency plots, but we don't actually seem to assign to self.fits, so I'm pretty sure this would crash. I might actually be responsible for this, I honestly don't remember anymore, but either way, it does look like this class hasn't been used with any fitting. Might be worth to comment out again or delete, unless you can be bothered to add the fitting yourself. As it's a resolution, the fit is simple as a Gaussian (symmetric), or an exponentiallly modified Gaussian (asymmetric), both of which ROOT's TMath has native support for, I believe.

benkrikler · 2018-03-19T09:20:02Z

cmsl1t/plotting/resolution.py

        for hist in normed_hists:
            if hist.integral != 0:
+                hist = hist / hist.integral()


There's something strange here in that integral seems to be working as both a function and an attribute on hist. If it's really a function, then I suppose line 81 will always return true. I think rootpy might be defining integral as a getter property, in which case I dont think calling it as a function will work. Not sure, but it's a bit confusing to me at least.

benkrikler · 2018-03-19T09:21:09Z

cmsl1t/plotting/resolution.py

+        normed_hists = [hist.Clone() for hist in hists]
+        for hist in normed_hists:
+            if hist.integral != 0:
+                hist = hist / hist.integral()


Same comment as above: is hist.integral a function or just a value (possibly returned as an @property)? Even if both are valid code, its nicer to stick to just one.

benkrikler · 2018-03-19T09:25:05Z

cmsl1t/plotting/resolution.py


    def __make_overlay(self, hists, fits, labels, ytitle, suffix=""):
        with preserve_current_style():
            # Draw each resolution (with fit)
+            # TODO: this feels like it does not belong here


I generally agree. The TitleOffset is less serious, but setting the range for all plots seems a bit too specific. As well as making it an option in the config file (somehow) we could at least put it as a parameter passed into the init method of this class.

benkrikler · 2018-03-19T09:33:59Z

cmsl1t/plotting/resolution.py

            for hist, label in zip(hists, labels):
                legend.AddEntry(hist, label)
            legend.SetBorderSize(0)
            legend.Draw()

+            ymax = 1.2 * hists[-1].GetMaximum()


How is hists sorted? Are we sure that the last one in the list will have the largest value? Might need to do something like:

ymax = 1.2 * max([hist.GetMaximum() for hist in hists])

…o cmsl1t.hist later)

kreczko · 2018-03-23T10:02:59Z

Added cmsl1t.math and cmsl1t.utils.hist to remove duplicated code from various analysers and plotting modules.

kreczko added 3 commits March 14, 2018 19:52

style changes to efficiencies

b047fa3

added suport for plotting rates

a17ce0c

updated style for resolution plots

10eaa27

kreczko requested a review from benkrikler March 16, 2018 11:24

benkrikler reviewed Mar 19, 2018

View reviewed changes

kreczko added 4 commits March 22, 2018 17:34

implemented Ben's feedback

3b8f8f5

added cmsl1t.math for cumulative sums

abe27be

added cmsl1t.utils.hist for histogram-related functions (might move t…

9cad233

…o cmsl1t.hist later)

replaced duplicated code with functions from cmsl1t.utils.hist

ed3650a

fixed unit tests

5e83b85

kreczko force-pushed the kreczko-aaron-alex-plotting branch from 26eca1f to 5e83b85 Compare March 23, 2018 13:49

kreczko merged commit dcf0276 into cms-l1t-offline:master Mar 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plotting updates from #106 #110

Plotting updates from #106 #110

kreczko commented Mar 14, 2018

kreczko commented Mar 14, 2018

benkrikler left a comment

benkrikler Mar 17, 2018

benkrikler Mar 19, 2018

kreczko Mar 22, 2018

benkrikler Mar 22, 2018

kreczko Mar 22, 2018

benkrikler Mar 22, 2018

kreczko Mar 26, 2018 •

edited

Loading

kreczko Mar 26, 2018

benkrikler Mar 19, 2018

kreczko Mar 22, 2018

kreczko Mar 23, 2018 •

edited

Loading

benkrikler Mar 23, 2018

kreczko Mar 23, 2018

benkrikler Mar 19, 2018

professor-calculus Mar 19, 2018

benkrikler Mar 19, 2018

kreczko Mar 22, 2018

benkrikler Mar 22, 2018 •

edited

Loading

kreczko Mar 22, 2018

benkrikler Mar 19, 2018

benkrikler Mar 19, 2018

benkrikler Mar 19, 2018

benkrikler Mar 19, 2018

benkrikler Mar 19, 2018

kreczko Mar 22, 2018

kreczko commented Mar 23, 2018

Plotting updates from #106 #110

Plotting updates from #106 #110

Conversation

kreczko commented Mar 14, 2018

kreczko commented Mar 14, 2018

benkrikler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kreczko Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kreczko Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benkrikler Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kreczko commented Mar 23, 2018

kreczko Mar 26, 2018 •

edited

Loading

kreczko Mar 23, 2018 •

edited

Loading

benkrikler Mar 22, 2018 •

edited

Loading