Institute Python code standard?

jared321 commented 4 months ago

On branch 85_FixCI, I have added the ability to run flake8 on all Python code in the repo using the tools script check_python_code.sh.

@asemposki @ominusliticus @jcyannotty Do you all want to keep this facility? If so, then I suppose that each developer will eventually need to clean up their code on a separate branch so that it is passing flake8. Once the code is passing, I can setup a CI action that runs the script on PRs.

ominusliticus commented 4 months ago

@jared321, up until now we have use autopep8 for code formatting. Would you mind adding a line to the shell script that auto formats the python files:

pip install autopep8   # for python to see autopep8 executable
autopep8 --in-line --aggressive --aggressive <file | directory>

jared321 commented 4 months ago

There are shades for doing this sort of work and you might want to offer more that one shade. Here are some that I am aware of

Make flake8 available (as a script or as a precommit hook?) so that users can manually fix up their code. Over time, you learn how to write standard-compliant code.
Make a reformatting tool such as autopep8 or black (more about formatting than style) available so that users can commit, autoupdate, and diff their code to see if they accept the changes.
Install a tool like autopep8 or black to be run automatically as part of a commit (possible?).

Personally I don't trust tools to alter my code and prefer (1). There's also a question of how much you want to force people to adhere to your standard or how strong it is. For instance, if all the code in the repo has been passed through autopep8, then it must be weaker than flake8 because flake8 is complaining. I don't like black because it wants all files to have very similar formatting. In some cases, it will rewrite my lines of code (e.g., math code) in such a way that it's hard for me to review it to check for correctness.

As an example of shades, we could leave the current script as if for wimps like me. I could add another script in that users could call in the spirit of (2) to reformat their code.

ominusliticus commented 4 months ago

I am a fan of standardizing the code formatting. Such formatting can take place during a precommit hook. But I am more than happy to sacrifice that as long as flake8 is strict with its requirements (which it is). My big insistence on formatting is to ensure that python files don’t have more than 79 columns of text (otherwise my linters go crazy)

Can you share a screen shot of the errors that flake8 is reporting, so we can establish where the errors are arising from. Up until now, it was only the develop branch that had the full code formatting (done with autopep8) done.

jared321 commented 4 months ago

flake8 seems to be a reasonable middle ground and as best I understand does what autopep8 does and more. It makes my life easier by finding some problems (as a compiler would), helps me standardize my code, but allows me some freedom to format in a way that improves code reviews.

The flake8 default is 79 columns. I have boosted it to 80, but will reset for you.

On my branch, I have setup things so that developers could easily run flake8. We would need to discuss this during a meeting, however, to get everyone up and running. Also, we should have a plan for how to address issues in an orderly way.

./docs/source/conf.py:24:80: E501 line too long (81 > 79 characters)
./src/Taweret/__init__.py:2:80: E501 line too long (88 > 79 characters)
./src/Taweret/__init__.py:3:80: E501 line too long (99 > 79 characters)
./src/Taweret/core/trees_setup.py:28:80: E501 line too long (114 > 79 characters)
./src/Taweret/core/trees_setup.py:46:80: E501 line too long (96 > 79 characters)
./src/Taweret/core/trees_setup.py:59:80: E501 line too long (96 > 79 characters)
./src/Taweret/core/trees_setup.py:82:80: E501 line too long (89 > 79 characters)
./src/Taweret/core/trees_setup.py:83:9: F841 local variable 'delete_out' is assigned to but never used
./src/Taweret/core/trees_setup.py:84:80: E501 line too long (90 > 79 characters)
./src/Taweret/core/wrappers.py:4:80: E501 line too long (103 > 79 characters)
./src/Taweret/core/wrappers.py:84:80: E501 line too long (84 > 79 characters)
./src/Taweret/core/wrappers.py:87:80: E501 line too long (102 > 79 characters)
./src/Taweret/mix/bivariate_linear.py:5:1: F401 'os' imported but unused
./src/Taweret/mix/bivariate_linear.py:6:1: F401 'shutil' imported but unused
./src/Taweret/mix/bivariate_linear.py:15:1: F401 'typing.Any' imported but unused
./src/Taweret/mix/bivariate_linear.py:16:1: F401 'typing.Callable' imported but unused
./src/Taweret/mix/bivariate_linear.py:20:1: F401 'typing.Tuple' imported but unused
./src/Taweret/mix/bivariate_linear.py:22:1: F401 'pathlib.Path' imported but unused
./src/Taweret/mix/bivariate_linear.py:290:9: F841 local variable 'n_samples' is assigned to but never used
./src/Taweret/mix/bivariate_linear.py:300:80: E501 line too long (99 > 79 characters)
./src/Taweret/mix/bivariate_linear.py:347:35: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
./src/Taweret/mix/bivariate_linear.py:502:25: F541 f-string is missing placeholders
./src/Taweret/mix/bivariate_linear.py:546:80: E501 line too long (122 > 79 characters)
./src/Taweret/mix/bivariate_linear.py:547:80: E501 line too long (106 > 79 characters)
./src/Taweret/mix/bivariate_linear.py:576:80: E501 line too long (99 > 79 characters)
./src/Taweret/mix/bivariate_linear.py:578:80: E501 line too long (99 > 79 characters)
./src/Taweret/mix/trees.py:9:1: F401 'logging.raiseExceptions' imported but unused
./src/Taweret/mix/trees.py:17:1: F401 'typing' imported but unused
./src/Taweret/mix/trees.py:19:1: F401 'scipy.stats.norm' imported but unused
./src/Taweret/mix/trees.py:29:80: E501 line too long (88 > 79 characters)
./src/Taweret/mix/trees.py:30:80: E501 line too long (89 > 79 characters)
./src/Taweret/mix/trees.py:31:80: E501 line too long (92 > 79 characters)
./src/Taweret/mix/trees.py:49:80: E501 line too long (127 > 79 characters)
./src/Taweret/mix/trees.py:53:80: E501 line too long (80 > 79 characters)
./src/Taweret/mix/trees.py:238:80: E501 line too long (91 > 79 characters)
./src/Taweret/mix/trees.py:258:80: E501 line too long (82 > 79 characters)
./src/Taweret/mix/trees.py:281:80: E501 line too long (127 > 79 characters)
./src/Taweret/mix/trees.py:337:80: E501 line too long (87 > 79 characters)
./src/Taweret/mix/trees.py:338:80: E501 line too long (84 > 79 characters)
./src/Taweret/mix/trees.py:339:80: E501 line too long (82 > 79 characters)
./src/Taweret/mix/trees.py:434:80: E501 line too long (92 > 79 characters)
./src/Taweret/mix/trees.py:448:80: E501 line too long (80 > 79 characters)
./src/Taweret/mix/trees.py:454:80: E501 line too long (92 > 79 characters)
./src/Taweret/mix/trees.py:460:80: E501 line too long (99 > 79 characters)
./src/Taweret/mix/trees.py:461:80: E501 line too long (80 > 79 characters)
./src/Taweret/mix/trees.py:463:80: E501 line too long (94 > 79 characters)
./src/Taweret/mix/trees.py:523:80: E501 line too long (85 > 79 characters)
./src/Taweret/mix/trees.py:546:80: E501 line too long (92 > 79 characters)
./src/Taweret/mix/trees.py:552:80: E501 line too long (86 > 79 characters)
./src/Taweret/mix/trees.py:553:80: E501 line too long (81 > 79 characters)
./src/Taweret/mix/trees.py:555:80: E501 line too long (95 > 79 characters)
./src/Taweret/mix/trees.py:566:80: E501 line too long (110 > 79 characters)
./src/Taweret/mix/trees.py:639:13: F841 local variable 'out_pred' is assigned to but never used
./src/Taweret/mix/trees.py:643:9: F841 local variable 'fig' is assigned to but never used
./src/Taweret/mix/trees.py:680:13: F841 local variable 'out_wts' is assigned to but never used
./src/Taweret/mix/trees.py:684:9: F841 local variable 'fig' is assigned to but never used
./src/Taweret/mix/trees.py:703:9: F841 local variable 'fig' is assigned to but never used
./src/Taweret/mix/trees.py:846:31: E225 missing whitespace around operator
./src/Taweret/mix/trees.py:858:80: E501 line too long (135 > 79 characters)
./src/Taweret/mix/trees.py:872:80: E501 line too long (84 > 79 characters)
./src/Taweret/mix/trees.py:878:17: E265 block comment should start with '# '
./src/Taweret/mix/trees.py:878:80: E501 line too long (136 > 79 characters)
./src/Taweret/mix/trees.py:879:17: E265 block comment should start with '# '
./src/Taweret/mix/trees.py:879:80: E501 line too long (144 > 79 characters)
./src/Taweret/mix/trees.py:887:41: E127 continuation line over-indented for visual indent
./src/Taweret/mix/trees.py:899:41: E128 continuation line under-indented for visual indent
./src/Taweret/mix/trees.py:900:41: E128 continuation line under-indented for visual indent
./src/Taweret/mix/trees.py:901:41: E128 continuation line under-indented for visual indent
./src/Taweret/mix/trees.py:906:80: E501 line too long (84 > 79 characters)
./src/Taweret/mix/trees.py:967:80: E501 line too long (116 > 79 characters)
./src/Taweret/mix/trees.py:999:80: E501 line too long (96 > 79 characters)
./src/Taweret/mix/trees.py:1083:80: E501 line too long (80 > 79 characters)
./src/Taweret/mix/trees.py:1095:80: E501 line too long (81 > 79 characters)
./src/Taweret/mix/trees.py:1097:1: W391 blank line at end of file
./src/Taweret/models/coleman_models.py:3:80: E501 line too long (101 > 79 characters)
./src/Taweret/models/coleman_models.py:4:1: F401 'Taweret.utils.utils.normed_mvn_loglike' imported but unused
./src/Taweret/models/coleman_models.py:52:80: E501 line too long (81 > 79 characters)
./src/Taweret/models/coleman_models.py:72:80: E501 line too long (81 > 79 characters)
./src/Taweret/models/coleman_models.py:73:80: E501 line too long (82 > 79 characters)
./src/Taweret/models/coleman_models.py:163:80: E501 line too long (81 > 79 characters)
./src/Taweret/models/coleman_models.py:183:80: E501 line too long (81 > 79 characters)
./src/Taweret/models/coleman_models.py:184:80: E501 line too long (82 > 79 characters)
./src/Taweret/models/polynomial_models.py:42:80: E501 line too long (80 > 79 characters)
./src/Taweret/models/polynomial_models.py:100:80: E501 line too long (80 > 79 characters)
./src/Taweret/models/polynomial_models.py:191:80: E501 line too long (80 > 79 characters)
./src/Taweret/models/polynomial_models.py:293:80: E501 line too long (80 > 79 characters)
./src/Taweret/models/samba_models.py:9:80: E501 line too long (101 > 79 characters)
./src/Taweret/models/samba_models.py:89:80: E501 line too long (103 > 79 characters)
./src/Taweret/models/samba_models.py:132:80: E501 line too long (80 > 79 characters)
./src/Taweret/models/samba_models.py:133:80: E501 line too long (82 > 79 characters)
./src/Taweret/models/samba_models.py:156:80: E501 line too long (95 > 79 characters)
./src/Taweret/models/samba_models.py:157:80: E501 line too long (95 > 79 characters)
./src/Taweret/models/samba_models.py:167:80: E501 line too long (87 > 79 characters)
./src/Taweret/models/samba_models.py:181:80: E501 line too long (80 > 79 characters)
./src/Taweret/models/samba_models.py:182:80: E501 line too long (82 > 79 characters)
./src/Taweret/models/samba_models.py:205:80: E501 line too long (95 > 79 characters)
./src/Taweret/models/samba_models.py:206:80: E501 line too long (95 > 79 characters)
./src/Taweret/models/samba_models.py:216:80: E501 line too long (87 > 79 characters)
./src/Taweret/models/samba_models.py:347:80: E501 line too long (83 > 79 characters)
./src/Taweret/models/samba_models.py:348:80: E501 line too long (96 > 79 characters)
./src/Taweret/models/samba_models.py:356:80: E501 line too long (85 > 79 characters)
./src/Taweret/models/samba_models.py:362:80: E501 line too long (80 > 79 characters)
./src/Taweret/models/samba_models.py:362:80: E502 the backslash is redundant between brackets
./src/Taweret/models/samba_models.py:363:21: E128 continuation line under-indented for visual indent
./src/Taweret/models/samba_models.py:363:80: E501 line too long (82 > 79 characters)
./src/Taweret/models/samba_models.py:464:80: E501 line too long (84 > 79 characters)
./src/Taweret/sampler/likelihood_wrappers.py:19:80: E501 line too long (81 > 79 characters)
./src/Taweret/sampler/likelihood_wrappers.py:20:80: E501 line too long (101 > 79 characters)
./src/Taweret/sampler/likelihood_wrappers.py:25:80: E501 line too long (95 > 79 characters)
./src/Taweret/tests/test_bivariate_linear.py:23:1: E402 module level import not at top of file
./src/Taweret/tests/test_bivariate_linear.py:24:1: F401 'pytest' imported but unused
./src/Taweret/tests/test_bivariate_linear.py:24:1: E402 module level import not at top of file
./src/Taweret/tests/test_bivariate_linear.py:25:1: E402 module level import not at top of file
./src/Taweret/tests/test_bivariate_linear.py:26:1: E402 module level import not at top of file
./src/Taweret/tests/test_bivariate_linear.py:27:1: F811 redefinition of unused 'approx' from line 23
./src/Taweret/tests/test_bivariate_linear.py:27:1: E402 module level import not at top of file
./src/Taweret/tests/test_bivariate_linear.py:28:1: E402 module level import not at top of file
./src/Taweret/tests/test_bivariate_linear.py:29:1: E402 module level import not at top of file
./src/Taweret/tests/test_bivariate_linear.py:146:4: E114 indentation is not a multiple of 4 (comment)
./src/Taweret/tests/test_bivariate_linear.py:190:5: F841 local variable 'log_like_from_test' is assigned to but never used
./src/Taweret/tests/test_bivariate_linear.py:209:5: F841 local variable 'log_like_from_test' is assigned to but never used
./src/Taweret/tests/test_bivariate_linear.py:228:5: F841 local variable 'log_like_from_test' is assigned to but never used
./src/Taweret/tests/test_bivariate_linear.py:247:80: E501 line too long (80 > 79 characters)
./src/Taweret/tests/test_bivariate_linear.py:249:1: W391 blank line at end of file
./src/Taweret/tests/test_gaussian.py:18:1: F403 'from Taweret.core.base_model import *' used; unable to detect undefined names
./src/Taweret/tests/test_gaussian.py:18:1: E402 module level import not at top of file
./src/Taweret/tests/test_gaussian.py:19:1: F403 'from Taweret.core.base_mixer import *' used; unable to detect undefined names
./src/Taweret/tests/test_gaussian.py:19:1: E402 module level import not at top of file
./src/Taweret/tests/test_gaussian.py:20:1: F403 'from Taweret.mix.gaussian import *' used; unable to detect undefined names
./src/Taweret/tests/test_gaussian.py:20:1: E402 module level import not at top of file
./src/Taweret/tests/test_gaussian.py:21:1: F403 'from Taweret.models.samba_models import *' used; unable to detect undefined names
./src/Taweret/tests/test_gaussian.py:21:1: E402 module level import not at top of file
./src/Taweret/tests/test_gaussian.py:22:1: F401 'pytest' imported but unused
./src/Taweret/tests/test_gaussian.py:22:1: E402 module level import not at top of file
./src/Taweret/tests/test_gaussian.py:23:1: E402 module level import not at top of file
./src/Taweret/tests/test_gaussian.py:33:11: F405 'Loworder' may be undefined, or defined from star imports: Taweret.core.base_mixer, Taweret.core.base_model, Taweret.mix.gaussian, Taweret.models.samba_models
./src/Taweret/tests/test_gaussian.py:34:11: F405 'Highorder' may be undefined, or defined from star imports: Taweret.core.base_mixer, Taweret.core.base_model, Taweret.mix.gaussian, Taweret.models.samba_models
./src/Taweret/tests/test_gaussian.py:52:9: F405 'Multivariate' may be undefined, or defined from star imports: Taweret.core.base_mixer, Taweret.core.base_model, Taweret.mix.gaussian, Taweret.models.samba_models
./src/Taweret/tests/test_gaussian.py:167:1: W391 blank line at end of file
./src/Taweret/tests/test_trees.py:33:1: E402 module level import not at top of file
./src/Taweret/tests/test_trees.py:34:1: E402 module level import not at top of file
./src/Taweret/tests/test_trees.py:46:80: E501 line too long (80 > 79 characters)
./src/Taweret/tests/test_trees.py:71:80: E501 line too long (81 > 79 characters)
./src/Taweret/tests/test_trees.py:72:29: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
./src/Taweret/tests/test_trees.py:75:5: F841 local variable 'fit' is assigned to but never used
./src/Taweret/tests/test_trees.py:98:5: F841 local variable 'f0_test' is assigned to but never used
./src/Taweret/tests/test_trees.py:150:80: E501 line too long (101 > 79 characters)
./src/Taweret/utils/utils.py:2:1: F401 'logging.raiseExceptions' imported but unused
./src/Taweret/utils/utils.py:158:80: E501 line too long (85 > 79 characters)
./src/Taweret/utils/utils.py:221:80: E501 line too long (82 > 79 characters)
./src/Taweret/utils/utils.py:232:80: E501 line too long (82 > 79 characters)
./src/Taweret/utils/utils.py:246:80: E501 line too long (82 > 79 characters)
./src/Taweret/utils/utils.py:251:80: E501 line too long (87 > 79 characters)
./src/Taweret/utils/utils.py:274:80: E501 line too long (111 > 79 characters)

ominusliticus commented 4 months ago

Thank you for sharing that! The "module level import not at top of file" is one that often shows up in our test files since we manually add the Taweret source dir to the path. Hopefully, by moving the tests into the Taweret source tree, we won't need to modify the path anymore.

Regarding the standardizing of issues (contributions, PR-ing, commit message ettiquette, and branch/merging practices) is of big importance to me, but I am poor at implementing myself. It will be good to have someone spearhead that!

jared321 commented 4 months ago

Great. Hopefully we can start nibbling away at some of those as we go along.

jared321 commented 4 weeks ago

We now have flake8 integrated into tox and it is run as a GH action on all Python code. All code incompatibilities reported by flake8 were eventually cleaned up. Closed.

bandframework / Taweret

Institute Python code standard? #87