Applied-GeoSolutions / multitemporal

Efficient, chainable time series processing of raster stacks.
GNU General Public License v2.0
4 stars 3 forks source link

update to python 3 #43

Closed ra-tolson closed 4 years ago

ra-tolson commented 4 years ago

looks like I need to start by updating the Dockerfile and test suite (which only has one test but it could still be useful).

After that I'll look at doing one of the 2-to-3 conversion processes.

bhbraswell commented 4 years ago

For what it's worth I did a rush job of this a while back and only had to change the container to get it to work.

ra-tolson commented 4 years ago

That's good to know; I may pester you with questions if I get stuck. :smiley_cat:

ra-tolson commented 4 years ago

Now that I have a test that passes I'd be interested to know what the container change was, if wouldn't mind pasting a diff or something.

bhbraswell commented 4 years ago

multitemporal.diff.txt

ra-tolson commented 4 years ago

Did you do anything else by any chance? Looks like you would've had to port all the source code too, probably?

E     File "/multitemporal/multitemporal/mt.py", line 149
E       print "there are no data paths for %s" % projdir
E                                            ^
E   SyntaxError: Missing parentheses in call to 'print'. Did you mean print("there are no data paths for %s" % projdir)?
bhbraswell commented 4 years ago

Wow, yeah, of course. I'll see if I've got some branch problems.

ra-tolson commented 4 years ago

If you want a branch to play with in python 3, I just pushed 43-py3; it passes the single test (which tests the slightly rewritten passthrough module). The test passes in both python 2 and 3(!).

Interestingly I didn't convert it by hand; I used futurize: https://python-future.org/automatic_conversion.html

You can see the past couple commits showing the conversion process. That's not normal python 3 code by any stretch, but keeping it that way depends on if we care about maintaining python 2 compatibility.

ra-tolson commented 4 years ago

PS test coverage:

----------- coverage: platform linux, python 3.6.7-final-0 -----------
Name                                 Stmts   Miss  Cover
--------------------------------------------------------
multitemporal/__init__.py                1      0   100%
multitemporal/bin/__init__.py            0      0   100%
multitemporal/bin/example.py            15     15     0%
multitemporal/bin/setup.py               6      6     0%
multitemporal/bin/testsharedmem.py      40     40     0%
multitemporal/mt.py                    305     94    69%
multitemporal/test/__init__.py           0      0   100%
multitemporal/test/test_1.py            21      0   100%
multitemporal/version.py                 1      0   100%
--------------------------------------------------------
TOTAL                                  389    155    60%
ra-tolson commented 4 years ago

So with that PR the test coverage will be much higher; I looked through the coverage line by line so I didn't generate another percentage though. I also ran the code interactively to exercise all the arg parsing bits in main().

ircwaves commented 4 years ago

should setup.py also get a language_level=3 specification as part of this? I saw this warning earlier today:

/usr/local/lib/python2.7/dist-packages/Cython/Compiler/Main.py:369: FutureWarning: \
   Cython directive 'language_level' not set, using 2 for now (Py2). \
   This will change in a later release! File: some_awesome_module.pyx
ra-tolson commented 4 years ago

If you mean adding python_requires='>=3', to setup(), I feel like that's a good idea, and it looks like related metadata still says python 2 as well. I'll add a commit to the PR.

However, the cython language level is set to 2 because the cython source is unchanged. I've been under the impression that changing the cython was out of scope because cython version 2 would continue to be supported.