wptmdoorn / methcomp

Python package providing functionality and plotting for chemistry method comparison
MIT License
14 stars 9 forks source link

Linear regression class accesses intercept_stderror from scipy (py 3.6 <= problems) #15

Closed wptmdoorn closed 3 years ago

wptmdoorn commented 3 years ago

The new Linear regression class uses intercept_stderr from scipy.stats which was implemented in their v1.6.0 release (https://github.com/scipy/scipy/pull/12983). The v1.6.0 release is unfortunately not available for Python 3.6 or lower - which means we either need to drop Python 3.6 support, or create some work arounds designed for Python 3.6.

Thoughts? @thomasburgess

Also see: https://travis-ci.org/github/wptmdoorn/methcomp/jobs/772064968 https://travis-ci.org/github/wptmdoorn/methcomp/jobs/772064969

thomasburgess commented 3 years ago

I looked into calculating it myself, but then we have to reimplement the while regression which seems a little silly. Statsmodels causes warnings with python 3.8+ which is why I was thinking of getting rid of it. I feel perhaps it is not unreasonable to keep up with the minimal versions required by scipy.

thomasburgess commented 3 years ago

On the other hand, if this is all that keeps us from 3.6:


intercept_stderr = stderr * np.sqrt(np.var(x) + x.mean()**2)

I'll give it a try

thomasburgess commented 3 years ago

Ok this is now in branch feature/precommit - I am not 100% it works, and test will require a mock - please try it

        # Hack to support scipy < 1.60
        if "intercept_stderr" not in result:
            result["intercept_stderr"] = result["stderr"] * np.sqrt(
                np.var(self.method1) + self.method1.mean() ** 2
            )
wptmdoorn commented 3 years ago

Thanks, I see you kind of adopted the scipy.stats implementation of the intercept_stderr. I will test it and see if it returns the same results. I think this is an appropriate solution for the solution, as dropping <py3.7 support already is probably a bit too early.

Will come back to you.

thomasburgess commented 3 years ago

As the calculation was so simple, I figure it is ok to borrow one line from scipy... for bigger chunks I wouldn't advise doing this

wptmdoorn commented 3 years ago

Yes, it looks good. I will review pre-commit branch now and if we merge it we can close this issue.

thomasburgess commented 3 years ago

Note: i had to add pyproject.toml, not sure how that interacts with setup.py, better check dist works as intended... On 24. May 2021, 12:53 +0200, William van Doorn @.***>, wrote:

Yes, it looks good. I will review pre-commit branch now and if we merge it we can close this issue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

thomasburgess commented 3 years ago

Ok, with travis finally happy, I guess we can close this guy now