Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

python 2 -> 3 #64

Closed
benjimin opened this issue Mar 19, 2019 · 13 comments
Closed

python 2 -> 3 #64

benjimin opened this issue Mar 19, 2019 · 13 comments

Comments

@benjimin
Copy link
Collaborator

benjimin commented Mar 19, 2019

Python 2 (and associated libraries/dependencies) will be unmaintained/unsupported about 9 months from now; upgrading also provides various improvements and compatibility.

TODO:

  • try automatic 2to3 conversion
  • may need to manually replace basemap with cartopy (3-4 plots?)

Then confirm it still works (e.g. automated tests pass)

Doing this prior to hazimp integration would simplify versioning.

@GrahamMore
Copy link

Hi benjimin,
I've recently came across this excellent library and I've been attempting to move it from 2 to 3. I've managed to get 95% of the tests to pass and get sensible results when running the single storm event. I'm currently trying to get the C files to compile to start testing the main monte-carlo process.
I'll commit the changes to my fork soon.

@benjimin
Copy link
Collaborator Author

Another potential complication is the necessary update from ConfigParser to configparser (with side-benefit of code simplification by its "fallback" argument), which currently triggers a metaclass conflict (in python 2 at least) upon multiple-inheritance with a Singleton class.. (Instead of fixing this particular complication, it could be bypassed with refactors to also address issues #61 and #68..)

@benjimin
Copy link
Collaborator Author

I've just pushed some changes (e.g. commit 90e6da0 to hazimp branch) that bypass the configparser/singleton/metaclass conflict.

@benjimin
Copy link
Collaborator Author

Checked; a trivial 2to3 auto-conversion does not immediately pass existing tests:

  • 30 import errors:
    • 18 when decoding a str in tests/pathLocate:getRootDirectory (TypeError)
    • 9 when instantiating _ConfigParser and reading defaults (str/bytes TypeError)
    • 3 concerning Basemap in PlotInterface (KeyError)
  • 6 involving pickle (str/bytes TypeError)
  • 2 errors specific to tests of the Singleton class

@GrahamMore how have your changes been going?

@GrahamMore
Copy link

GrahamMore commented Apr 16, 2019

@benjimin - I've pushed some changes to my forked repo. Excuse my ignorance to commonplace coding practises, as I'm not a full time programmer so I'm more used to hacking stuff together rather than formal version controlling, structuring etc.

So far I've got all the tests to pass, bar those in TestRandom that I'm trying to sort out.
I've tried to swap to using the randomgen package for the random number generator as the jumphead function (used in TrackGenerator) is no longer within the python 3 version of the Random package. Switching to the PCG64 from the randomgen package that may to handle parallelism better-https://bashtage.github.io/randomgen/parallel.html

I had tried to rerun the validation tests with randomgen in place and using their Mersenne Twister algorithm, that should be consistent with the existing implementation in Random, but for the same seed of 1 I obtained different results. I suspect this might be due to a between the representation of 1 between Py 2 (C int/ python long) and 3 ( python long only), but I'm not sure,

I'm in progress of building a test to see if the overall histrogram created from the Random vs randomgen for the same seed is generally consistent.

I've updated the requirements.txt file so if you wanted to clone my fork and try running the code it should be possible.

Cheers,
Graham

@benjimin
Copy link
Collaborator Author

@GrahamMore did you get the tests to pass?

I saw your changes looked fairly extensive, and I'd be interested if you're able to give a bit more detailed explanation (e.g. which automated commands you applied, and which parts you chose to change manually)?

@GrahamMore
Copy link

@benjimin
Almost all were manual amendments, slowly working through each test in turn to resolve the errors. Then working through the examples to understand if they gave plausible results to find additional errors not flagged by the tests.
Sadly I've not made any progress recently, I'll hopefully make some once work is slightly less manic.

@benjimin
Copy link
Collaborator Author

benjimin commented Jun 28, 2019

Hi @GrahamMore

We now have a version (currently in the "py3" branch) which passes all tests under python 3. Thanks! I didn't merge the entirety of your changes verbatim (because they were extensive with limited explanatory detail and because they were branched from an older release compared to the main development branch), nonetheless I've been finding them extremely helpful!

Hopefully after a few weeks (and some additional tests) we'll update an official release.

@GrahamMore
Copy link

Hi @benjimin, excellent effort!
I'll try to find some time in the coming weeks to go through comparing the changes to see if there are other that would still be valid. I'll make sure to amend them all on individual commits to make it all a bit more traceable.

@benjimin
Copy link
Collaborator Author

Update: a couple further fixes; both example scenarios should now be running (in addition to the tests). BTW @GrahamMore, I agree with your choice to use randomgen, as numpy appears to have also made the same choice!

@wcarthur
Copy link
Member

Need to port to mpi4py as PyPar not planning to transition to Python3.x (see daleroberts/pypar#13). I'll raies as a separate issue

@benjimin
Copy link
Collaborator Author

Could also consider using python standard library functions for multiprocessing (as no need for MPI specifically).

@wcarthur
Copy link
Member

wcarthur commented Feb 3, 2020

@GrahamMore can you please take a look at the new master branch (tag v3.0 095c427). I've implemented mpi4py for parallelisation and would appreciate any additional testing you might be able to do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants