search

joshspeagle / dynesty

Dynamic Nested Sampling package for computing Bayesian posteriors and evidences
https://dynesty.readthedocs.io/
MIT License
357 stars 77 forks source link

pylint failures #338

Closed joshspeagle closed 3 years ago

joshspeagle commented 3 years ago

Looks like tests are now failing under Run pylint --fail-under=8 --extension-pkg-whitelist=scipy.special py/dynesty/*py

segasai commented 3 years ago

weird... Let me take a look.

segasai commented 3 years ago

Okay, I see what's going on. Some combination of recent results changes and (possibly) some pylint changes now let it think that this line https://github.com/joshspeagle/dynesty/blob/b8a62c6f2e8e289ab37e39ca5cce4890dd6551a5/py/dynesty/dynamicsampler.py#L1849 has an error: "Instance of 'Results' has no 'logz' member"

I don't quite know how pylint was doing the inference before, so that was not flagged... I think this needs to be fixed properly (as opposed to lowering the threshold pylint score), as pylint is helpful to see actual bugs, so I'd rather avoid spurious errors in pylint output. But I'll need to understand better how pylint does the inference.

segasai commented 3 years ago

That brings another point, when I refactored merge_run, I saw the compute_aux option, but it wasn't really clear whether there is much value of it being off, i.e. results without logz, logwt. And because of that option, I couldn't force the Results() object to always contain those keywords, which I think would be helpful. I don't think the performance cost of computing logz/logzerr is significant enough to keep the compute_aux option, so I'd rather get rid of it (unless I miss something)

segasai commented 3 years ago

Fixed in 51ff47efbce719c9e4563d7696d0cdbea6bd3b2b by adding .pylintrc that excludes Results from checking, as it has dynamic attributes