flav-io / flavio

A Python package for flavour physics phenomenology in the Standard model and beyond
http://flav-io.github.io/
MIT License
71 stars 62 forks source link

Bug when loading covariance with several observables for a single observable FastFit #68

Closed MJKirk closed 6 years ago

MJKirk commented 6 years ago

Bug introduced in pull request #48 -- there is an edge case that if we using FastFit with only a single observable, but are loading a covariance with multiple observables, we get the whole covariance matrix instead of a single element. Fix by checking the size of the covariance matrix being loaded.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.007%) to 93.784% when pulling 856918b50bdcd9ebaa389590014de46c238c1083 on MJKirk:covariance_loading_bugfix into 4d8007d5a0dd456c527e23abe87a9494b6949db4 on flav-io:master.

DavidMStraub commented 6 years ago

Thanks, good point! Actually this also affects the new statistics.likelihood module that will be released with the next version (today, maybe?). This is meant as a replacement for statistics.fits but will probably coexist for a couple of versions. I will fix it there, too.

MJKirk commented 6 years ago

Small thing I noticed while checking my fix, the error message when the loaded covariance doesn't contain all the observables is slightly opaque. https://github.com/flav-io/flavio/blob/4e0b8623624a5d7d36c8df8a4dde32a19b5d9c96/flavio/statistics/fits.py#L591-L595 The string on line 594 doesn't get printed and the code continues to the next line, and so the only error you get is that permutation is not defined.

DavidMStraub commented 6 years ago

Oh, which Exception does it throw then, actually?

MJKirk commented 6 years ago

You get UnboundLocalError The full traceback is

Traceback (most recent call last):
  File "flavio_test.py", line 23, in <module>
    f_single_obs.load_sm_covariance("test_covar.p")
  File "/home/mkirk/Documents/Software/flavio/flavio/statistics/fits.py", line 665, in load_sm_covariance
    self.load_sm_covariance_dict(d=data)
  File "/home/mkirk/Documents/Software/flavio/flavio/statistics/fits.py", line 680, in load_sm_covariance_dict
    assert len(permutation) == len(self.observables), \
UnboundLocalError: local variable 'permutation' referenced before assignment

So nothing is shown about the covariance matrix missing an observable

DavidMStraub commented 6 years ago

Thanks! Nice talk :wink:

DavidMStraub commented 6 years ago

Oh, that fix actually doesn't make sense. Shouldn't code during talks.

MJKirk commented 6 years ago

https://github.com/MJKirk/flavio/commit/e7cbc0441918547a3a531d332d3bb89a593bc544 is what I did in my fork

DavidMStraub commented 6 years ago

Oh exactly, that's what one should do. I can add it.