snoplusuk / echidna

MIT License
4 stars 12 forks source link

Limit debug #147

Closed ashleyrback closed 8 years ago

ashleyrback commented 8 years ago

Supersedes #143

ashleyrback commented 8 years ago

Created a new PR that supersedes #143, so we can get this merged today.

ashleyrback commented 8 years ago

@EdLeming, there is also one extra commit I added this morning that just adds some further debugging information. And the unittest fixes.

EdLeming commented 8 years ago

Minor pep8:

echidna/fit/minimise.py:186:80: E501 line too long (80 > 79 characters) echidna/util/root_help.py:8:1: E402 module level import not at top of file echidna/util/strings.py:4:1: E302 expected 2 blank lines, found 1 echidna/util/strings.py:4:56: E502 the backslash is redundant between brackets echidna/util/strings.py:5:22: E127 continuation line over-indented for visual indent

That aside, it looks good. What's the thinking for changing per_bin to default to false in test statistic? Is this going to effect the fit.set_fit_result function? I can't see where that flag is passed.

ashleyrback commented 8 years ago

@EdLeming, that should fix the pep8 errors.

Which line are you talking about for the set_fit_results function?

EdLeming commented 8 years ago

Line 708 in fit_results - Shouldn't we pass the per_bin flag when creating the FitResults object

ashleyrback commented 8 years ago

Ah, I see what you mean for this line. Yes we should pass the correct per_bin flag. We probably also need to do this for the minimiser at this line.

I think the idea is that all the per_bin flags should match up:

should all have the same value for the per_bin flag. I this is the case everything should work. Therefore I would suggest that, where possible, things are passed the flag from the object creating them.

So we can pass self._per_bin when initialising Minimisers in Fit.

I'll add this now.

ashleyrback commented 8 years ago

Slight correction, FitResults doesn't need to be passed a per_bin flag as, in it's current form, it only works at the per_bin level anyway.

EdLeming commented 8 years ago

Sweet! Merging