CLN run isort on source code files

[chandan5362]

Solved this issue #4235
added isort for python files in .pre-commit-config.yaml

[MarcoGorelli]

Hi @chandan5362

Thanks for taking this on, but some of the changes seem strange to me - were you up-to-date with the latest changes from master when you started?

Could you try running:

git fetch --all --prune
git reset upstream/master
git add .pre-commit-config.yaml
git checkout -- .
pre-commit run isort --all-files
git add -u
git commit -m "sort imports"
git push origin isort-added -f

?

[MarcoGorelli]

Something's not quite right, you've ended up with extra lines of code which isort wouldn't add (e.g. in pymc3/backends/__init__.py).

Can you fetch from your upstream branch, do

git reset --hard upstream/master

, then make the same change you've made here to .pre-commit-config.yaml, then run pre-commit run isort -a, and then git add, git commit, git push -f ?

[MarcoGorelli]

Cool, thanks, now it looks right!

Test failure is due to the changes in pymc3/__init__.py, am just looking into it

[chandan5362]

Thanks to you as well for being so generous and helpful. Feels like I am getting into open source slowly and looking forward to contribute more. Thanks a lot @MarcoGorelli

[chandan5362]

I tried running pytest by explicitly importing theano.tensor.slinalg. It worked fine.
I found similar issue on Theano repo. I am attaching the link for your reference.
Please have a look into it

[MarcoGorelli]

OK, got it - currently, here's what happens:

in pymc3/__init__.py there's a call to from .backends.tracetab import *

this triggers pymc3/backends/__init__.py, in which there's a call to from ..backends.ndarray import NDArray, load_trace, point_list_to_multitrace, save_trace

in pymc3/backends/ndarray.py there's a call to from pymc3.backends import base

in pymc3/backends/base.py there's a call to from ..model import Model, modelcontext

in pymc3/model.py there's a call to from pymc3.math import flatten_list

in pymc3/math.py there's a call to import theano.tensor.slinalg.

And as you noted, import theano.tensor.slinalg needs to be run explicitly before importing from it.

So, just explicitly import theano.tensor.slinalg in pymc3/gp/util.py should be fine to solve the collection issue 🎉

[chandan5362]

Okay, Great!
cheers to myself:blush:
I am happy that I was helpful to you.

whether should I do the necessary changes and push it or you are going to resolve it yourself??

[MarcoGorelli]

If you just put import theano.tensor.slinalg inside pymc3/gp/util.py then I'd like to think it'll be enough. Perhaps run pre-commit run isort -a again just to make sure, then git add, git commit, and git push (-f shouldn't be necessary this time)

[MarcoGorelli]

you are going to resolve it yourself

just to be clear - I don't maintain this library, so I can't add commits to your PR

[MarcoGorelli]

just a couple of pylint errors now

---

I went through the rest and it looks good, cc @AlexAndorra

[MarcoGorelli]

you'll need to silence the pylint warning

Suggested change

	import theano.tensor.slinalg
	import theano.tensor.slinalg # pylint: disable=unused-import

[MarcoGorelli]

this line should probably only be applied where necessary (probably import scipy.sparse and some of the import theano.) rather than to the whole block

[MarcoGorelli]

pretty sure this shebang has no effect if it's not at the beginning of the file, so it can probably be removed

[codecov]

Codecov Report

Merging #4239 (3203195) into master (a5d1697) will decrease coverage by 0.07%.
The diff coverage is 97.76%.

@@            Coverage Diff             @@
##           master    #4239      +/-   ##
==========================================
- Coverage   87.64%   87.57%   -0.08%     
==========================================
  Files          88       88              
  Lines       14363    14274      -89     
==========================================
- Hits        12589    12500      -89     
  Misses       1774     1774              

Impacted Files	Coverage Δ
pymc3/distributions/shape_utils.py	100.00% <ø> (ø)
pymc3/plots/__init__.py	50.00% <ø> (ø)
pymc3/sampling_jax.py	0.00% <0.00%> (ø)
pymc3/step_methods/compound.py	95.65% <ø> (ø)
pymc3/step_methods/sgmcmc.py	0.00% <0.00%> (ø)
pymc3/step_methods/step_sizes.py	100.00% <ø> (ø)
pymc3/backends/__init__.py	100.00% <100.00%> (ø)
pymc3/backends/base.py	86.92% <100.00%> (ø)
pymc3/backends/ndarray.py	87.36% <100.00%> (ø)
pymc3/backends/report.py	90.90% <100.00%> (ø)
... and 67 more

[MarcoGorelli]

Thanks for updating - there's still some pylint warnings, if you run pylint --rcfile=.pylintrc pymc3/math.py you'll see them

Maybe it's better to keep the # pylint: disable=unused-import block then, just make sure that import scipy.sparse goes inside it (or, just add # pylint: disable=unused-import to that line)

The rest looks good anyway 👍 Takes a bit to review, but it only needs doing once

[AlexAndorra]

Thanks @chandan5362 and @MarcoGorelli, this looks good! I'll merge once the merge conflict in util.py is resolved

[chandan5362]

@AlexAndorra @MarcoGorelli I have resolved the merge conflict and pushed it. Please let me know, if any issue still persists with my PR.

[MarcoGorelli]

Hi @chandan5362 - looks like there's still a conflict, could you please fetch upstream/master, merge, and push again?

[MarcoGorelli]

Can you show the exact commands you ran to resolve the conflict? It looks like it's still present

[chandan5362]

Can you show the exact commands you ran to resolve the conflict? It looks like it's still present

mistakenly, instead of pushing it to remote, I pushed it to origin.
I am doing that again.

[michaelosthege]

@MarcoGorelli @chandan5362 I resolved the merge conflicts via the UI in Github.
I tried to get the import order right by inferring the rules, but I messed up.

Anyway - you can now pull the branch and at least the conflicts should be resolved.

[MarcoGorelli]

maybe we want to postpone this after the rush of getting PRs in for 3.10, else there'll be too many conflicts?

[MarcoGorelli]

make pre-commit happy

Doesn't look like this worked - what command did you run, @michaelosthege ? pre-commit run -a should be enough

[chandan5362]

As I can see that there are many conflicting files as of now. please let me know if there is anything that I have to do @MarcoGorelli

[MarcoGorelli]

As I can see that there are many conflicting files as of now. please let me know if there is anything that I have to do @MarcoGorelli

Hey @chandan5362 - yeah, you could either merge the conflicts, or you could just do a hard git reset to upstream/master, make the same changes you made here (# pylint: disable=unused-import, and importing theano.tensor.slinalg) and then run pre-commit run isort -a again. Do ping us if you want/need help with any of these steps

[Spaak]

Looks good to me! Thanks @chandan5362 and @MarcoGorelli!