search

cjekel / piecewise_linear_fit_py

fit piecewise linear data for a specified number of line segments
MIT License
289 stars 59 forks source link

drop python2, wrap repetitive code, minor edits #75

Closed vkhodygo closed 3 years ago

vkhodygo commented 3 years ago

Hi @cjekel

I was reading the code because I couldn't find some features in the documentation (they're there but too deep) and I noticed that some identical pieces are used through the code. I tried to make some simple functions to improve overall readability. I also did some minor editing, part of it is not compatible with python 2.7, don't you think it's time to drop it completely?

P.S. funny thing, while I was looking for your repo I found these shameless folks

cjekel commented 3 years ago

Hi Vladimir,

Thanks for the PR. Reducing the number of lines of code is great! and consolidating code is better! Yes, it's probably time to drop support for Python 2.7 :)

Not sure how I feel about dropping support for python 3.5... Let me think about this. In the meantime I'll get the repo ready to depreciate Python 2.7.

Funny thing I had no idea that other repo exisited! Thanks for pointing it to me. Seems like they do a weighted least squares, with assumptions on breakpoints. I know you were interested in weighted least squares methods at one point.

Cheers! CJ

cjekel commented 3 years ago

@h-vetinari Head's up for conda-forge. I'm planning on releasing a last version for Python 2.7. Then future version will have something like python_requires=">3.4" in the setup.py. Not sure if that will require a recipe change?

h-vetinari commented 3 years ago

@cjekel: @h-vetinari Head's up for conda-forge. I'm planning on releasing a last version for Python 2.7. Then future version will have something like python_requires=">3.4" in the setup.py. Not sure if that will require a recipe change?

Thanks, that won't be a problem, conda has not been building python 2.7 & 3.5 packages for a while now. The feedstock is currently building for 3.6-3.8 & pypy 3.6.

vkhodygo commented 3 years ago

That's good to hear! Sorry, I didn't think about v3.5, that's probably a bad habit of mine, I tend to use the latest stable release only. Should I add a deprecation warning for the time being?

Yeah, I was interested but that part in my thesis proved to be, well... Let's say my examiners do not share my views on the methods I used.

cjekel commented 3 years ago

@vkhodygo

Can you address h-vetinari's comments above? Otherwise LGTM :)

Should I add a deprecation warning for the time being?

Not necessary. I think adding the python_requires on the last release will be good enough, such that everything will still work for someone on an old Python release with pip install pwlf==2.0.4.

Yeah, I was interested but that part in my thesis proved to be, well... Let's say my examiners do not share my views on the methods I used.

Well you finished now? Congratulations! :tada:

vkhodygo commented 3 years ago

Fixed as you asked.

Well you finished now? Congratulations!

Thanks, I'm almost there - finishing some minor corrections as I've passed the viva.

cjekel commented 3 years ago

Thanks Vladimir!

@h-vetinari Thank you again for setting up the conda-forge builds and making this work more accessible to other users!