search

jgliss / pyplis

Python toolbox for the analysis of UV SO2 camera data
GNU General Public License v3.0
7 stars 5 forks source link

port to python 3 and lint #11

Closed johannjacobsohn closed 5 years ago

johannjacobsohn commented 5 years ago

This is a slightly bigger PR than I had in mind initially, but I've tried to port pyplis to python 3 and kinda fell into a rabbit hole... I've used flake8 quite extensively to find potential issues, and fixed all tests but one unit test. I've intentionally made it into a lot of commits for easier bug-hunting and bisecting later - but they can be squashed if you prefer. I also used pylint --py3k to find potential inconsistencies, and pydocstyle to make docstrings consistent (and make my editor stop screaming at me).

In theory, pyplis should now run on python 2 and 3 identically, or at least should be very close. Python 3 should have slightly better performance, and python 2 will be retired soon-ish (https://pythonclock.org/).

johannjacobsohn commented 5 years ago

please let me know if I can do anything to make this PR easier to digest

jgliss commented 5 years ago

@johannjacobsohn Many thanks for this initiative. I highly appreciate your efforts. Refactoring to Python 3 was something I had on my list since long, but since I can currently work on this project only occasionally in my spare time, I did not get that far yet.

I checked out your commits on my local machine and tried to run the example scripts but did not get very far until it crashed (on Python 2.7). I can test soon again and provide you with some more information about the errors. It is important that these scripts run through (and that the included tests pass), so we should try to get this working. I am sure, these are only minor issues, that can be fixed easily by running the scripts and debugging and I would appreciate your help here (I might be able to work on this around Christmas / new years).

Cheers, Jonas

johannjacobsohn commented 5 years ago

Hi Jonas,

I have already started to port/integrate these scripts. I have fixed all issues with scripts/RUN_INTRO_SCRIPTS.py but ran into quite a lot of problems with scripts/RUN_EXAMPLE_SCRIPTS.py. I hope to have this resolved today or in the next few days. I'll push the changes and ping you again.

Maybe it would be worthwhile to pull this into a py3-branch instead of master directly, so you can review and accept changes until everything is well-tested (edge-case issues might crop up in further testing, even though all tests and scripts pass).

jgliss commented 5 years ago

Hi Johann,

thank you! Great, that you are already on it. I'll wait until you get back to me then. I agree that it is a good idea to merge it into a separate py3 branch for now.

Cheers

johannjacobsohn commented 5 years ago

@jgliss This took a little bit longer than expected, but I found a number of issues and almost all scripts are working now (read: they don't crash), except for scripts/ex06_doas_calib.py and scripts/ex09_velo_optflow.py which seems to be related to the Gaussfit code. I speculate that this is because of the changed behaviour of divide between py2 and 3 (floordiv vs realdiv) and the changed rounding behaviour. Due to __future__, both py2 and py3 should behave the same, albeit failing the same atm. The good news is that ba541de is also solved and reverted, so all tests are passing now.

Can you take a look at 2216562? I can't figure it out. If you run $ tox it should run all tests and scripts, and succeed in both py27 and py36, and if you revert 2216562 it should fail again.

BTW, I can change the pull request to another branch, but you need to create that branch first.

jgliss commented 5 years ago

Hi Johann,

I merged your PR into the new py3 branch now and fixed some of the failing tests. Will check other failing tests soon (I think this is only minor stuff). Thanks again for your efforts and patience ;) I really appreciate your work and efforts. I hope I will find some more time in 2019 to maintain the software. Let me know, if you have suggestions or plans for future improvements and developments.

Happy new year!

Cheers, Jonas

solvejgdinger commented 5 years ago

Hi Johann, thanks for the great work and effort. I am eager to use your version and installed it in a new environment with python 3.7. The intro scripts run through, but I dont get further than example script 2. The function meas_geometry.find_viewing_direction() throws the error TypeError: can't pickle module objects tracing back to a copy.deepcopy() call. I am also failing 2 tests. Both, the pickle and copy library are installed within the scope of the standard library. You write that they are succeeding for you. Do you know if there are additionaly libraries (other than the specified depencies) required for python 3 or do you have an idea what could be problem?

johannjacobsohn commented 5 years ago

Hi @heliotropium72

I had that issue too, and it took me a really long time to figure out its a result from issues deepcopying geonum. I tried to fix that in https://github.com/jgliss/geonum/pull/2 and that has been merged in https://github.com/jgliss/geonum/commit/316868bc2ffef619a48500a2b5438f52138185c0 so make sure you use the most up-to-date version of geonum.

I think I mentioned in that pull request, but it might be worth repeating: this deepcopy issue confused me a lot, so I am not entirely sure my fix is actually the correct one. Seems to work, but might mess up something else. If you understand this better, please feel free to review that PR again. Or help me write better unit tests for geonum, which I tried but failed to do.

johannjacobsohn commented 5 years ago

also, I did not test py37, only py36. Shouldn't make a difference, but probably worth adding to tox.ini. I'll try and add that tonight! requirements.txt needs an update as well, so I'll add that too.

solvejgdinger commented 5 years ago

Thanks for pointing this out! I used pip install geonum. So there needs to be either a new version release to pip or some added notes in the readme of pyplis. I will have a look at your PR but I am not using geonum personally, so I am not entirely sure what it does.

All example scripts execute correctly, including scripts/ex06_doas_calib.py and scripts/ex09_velo_optflow.py when I use the original numbers from the master branch (ignoring 2216562). But I cannot explain why it works in my installation and not in yours.

However, pyplis\test\test_image_module.py fails with TypeError: 'str' object is not callable. But when I go through the test script line by line, all tests pass. I have no clue, what the problem could be. I will try to figure this out.

johannjacobsohn commented 5 years ago

I get the same error message in py37 which was raised in .tox/py37/lib/python3.7/site-packages/_pytest/fixtures.py I venture to guess that this is because I fixed pytest to version 3, because the handling of fixtures has changed since then and is incompatible with the current test code. And pytest 3 seems to be incompatible w/ py37. I'll look into it. In the meantime, I suggest you use python 3.6.

solvejgdinger commented 5 years ago

You were right again ;-) I fixed the test scripts and opened a new PR #15 . That should work with pytest 4 as well. I am not familiar with tox and have only tested in two local anaconda environments (python 2.7 and 3.6).

johannjacobsohn commented 5 years ago

tox will create virtual environments for you and run tests in them automatically, which makes it fairly elegant for multi-version testing. Just install tox and run $ tox in the pyplis root directory. It'll take a while, but will test everything. I've started to add py37 but ran into some tricky issues, which I think are related to braking changes between py36 and py37. I'll open a new PR against the py3 branch.

johannjacobsohn commented 5 years ago

Also, tox will recreate and re-install everything from requirements.txt from scratch, so you can be sure that file is complete and correct.

johannjacobsohn commented 5 years ago

However, pyplis\test\test_image_module.py fails with TypeError: 'str' object is not callable. But when I go through the test script line by line, all tests pass. I have no clue, what the problem could be. I will try to figure this out.

That was because pyplis overwrote __dir__ in that script, which crashes pytest. Fixed in #16

johannjacobsohn commented 5 years ago

rebased PR to new py3 branch, now only shows latest changes

jgliss commented 5 years ago

@johannjacobsohn I just went through the history of this PR and saw that you added some things after I merged into py3 on 2 January. Anyways, I merged py3 into master now and am planning the release of version 1.4.0 within this week. I would appreciate if you would checkout the current master and see, which of your recent updates that you submitted here got lost on the way and should be merged into master for v1.4.0, and send me an updated PR (or move this one back to master). I want to close this PR with release of v1.4.0. Thanks!

johannjacobsohn commented 5 years ago

jeah, i've taged on some cleanup commits... I'll get on it to get this resolved