Format update #35

marcomangano · 2020-10-31T10:03:03Z

Purpose

I re-formatted the code, as per this issue.
The first two commits are the black and manual flake8 fixes respectively.
I committed separately a few fixes that I would like the reviewers to double check carefully, to be sure that the code behaviour is not changing

Type of change

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (non-backwards-compatible fix or feature)
Code style update (formatting, renaming)
Refactoring (no functional changes, no API changes)
Documentation update
Maintenance update
Other (please describe)

Checklist

I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
I have run unit and regression tests which pass locally with my changes
I have added new tests that prove my fix is effective or that my feature works
I have added necessary documentation

ewu63 · 2020-10-31T15:03:10Z

src/complexify.py

+import os
+import glob
+import re
+from stat import *  # noqa: F403


Could you not tell what the star imports were?

Never mind I see you fixed this later on.

ewu63 · 2020-10-31T15:03:28Z

reg_tests/mdo_regression_helper.py

@@ -148,9 +148,9 @@ def reg_file_comp(ref_file, comp_file):
    str1 = "@value    3.141592653589793 1e-12 1e-12"
    str2 = "@value    3.141592653589999 1e-12 1e-12"

-    print("This comp should be True: ", reg_comp(str1, str2))
+    print("This comp should be True: ", _reg_str_comp(str1, str2))


I forget, why is this change necessary?

reg_comp does not exist anymore - I believed you renamed it to _reg_str_comp, but this was not caught as an error because we never run python mdo_regression_helper.py

ewu63 · 2020-10-31T15:05:04Z

idwarp/MultiUnstructuredMesh.py

 import numpy as np
 from pprint import pprint
 from mpi4py import MPI
 from .MExt import MExt
-from petsc4py import PETSc

 try:
    from cgnsutilities import cgns_utils as cs


This actually reminds me, we changed the file names, so this should probably be updated to
from cgnsutilities import cgnsutilities as cs

ewu63 · 2020-10-31T15:06:01Z

src/complexify.py

@@ -277,7 +277,6 @@ def write_output(filename, lines):
    try:
        g = open(newname, "w")
    except IOError as err:
-        f.close()


I suspect this was supposed to be g.close() I think? Not really sure.

Yes, that was my guess too... but then I realized that it did not make sense to close something that was never opened, so I removed it altogether. I can put g.close() back in, but one way or the other that except is going to exit the run - more or less gracefully

ewu63 · 2020-10-31T15:08:25Z

So I am a tiny bit uneasy about fixing complexify.py just because it's so old it needs a total overhaul. What I would recommend is reverting those files to how they were, and if you add a .flake8 file just to ignore those files, then Travis will work its magic. We can fix these at a later date.

See for example this file.

marcomangano · 2020-10-31T15:18:50Z

So I am a tiny bit uneasy about fixing complexify.py just because it's so old it needs a total overhaul. What I would recommend is reverting those files to how they were, and if you add a .flake8 file just to ignore those files, then Travis will work its magic. We can fix these at a later date.

See for example this file.

Yeah, I was a bit uneasy too but I gave it a shot. I will revert it back as you suggest

marcomangano · 2020-10-31T15:25:49Z

I will fix complexify by the end of the day

ewu63 · 2020-10-31T15:26:29Z

Sure, instead of reverting commits you can just copy/paste what we had before and do a single commit that way.

marcomangano · 2020-10-31T15:33:05Z

Sure, instead of reverting commits you can just copy/paste what we had before and do a single commit that way.

That's the only way I am able to do it ahaha

marcomangano · 2020-10-31T15:45:07Z

@nwu63 this should fix it?

ewu63 · 2020-10-31T15:47:02Z

Good except you still need to run black on it

ewu63

Good with me but I'd prefer someone else to also take a look.

eirikurj

Few minor things, otherwise looks good.

eirikurj · 2020-11-02T11:21:06Z

idwarp/MultiUnstructuredMesh.py

@@ -121,7 +124,7 @@ def __init__(self, CGNSFile, optionsDict, comm=None, dtype='d'):
            self.warp
        except AttributeError:
            curDir = os.path.dirname(os.path.realpath(__file__))
-            self.warp = MExt('idwarp', [curDir], debug=debug)._module
+            self.warp = MExt("idwarp", [curDir])._module


To maintain consistency please keep this argument and add a default value debug=False in the __init__ function.

eirikurj · 2020-11-02T11:23:01Z

idwarp/MultiUnstructuredMesh.py

@@ -763,12 +762,12 @@ def warpMesh(self):
        """

        # Get proc ID
-        myID = self.comm.Get_rank()
+        self.myID = self.comm.Get_rank()


Instead of overwriting the value (which is set once in the __init__ function) please just remove this line (and the ones below).

eirikurj · 2020-11-02T11:27:15Z

idwarp/MultiUnstructuredMesh.py

-                print('|     All IDWarp Options:               |')
-                print('+---------------------------------------+')
+                print("")
+                print(" Options of mesh", mesh, "of", len(self.meshes))


This will print out the handle to the mesh object. Not sure this is the desired behavior.

eirikurj

Few minor things, otherwise looks good. Do we want to add the black/flake8 commit to a git blame ignore file?

marcomangano · 2020-11-02T11:51:56Z

Few minor things, otherwise looks good. Do we want to add the black/flake8 commit to a git blame ignore file?

@nwu63 what is our final position on this? I have one I can add now, but we realized that such file is only useful locally.. other ppl must initialize it in their git option for it to work, and still it is not effective on the browser interface. Your call guys!

ewu63 · 2020-11-02T14:42:48Z

Yeah personally I don't see much value in it since git cannot pick it out automatically, and it just sort of clutters the repo. Most tools can git blame multiple times and as long as your commit message is clear I think it's fine.

marcomangano added 7 commits October 31, 2020 10:09

reformatting with black

b13e7e8

flake8 fixes, removed unused imps and vars

80aa1dd

I believe that was not supposed to be there

2dd72d3

removed unused stat import

1bfc802

removed wrong input option

c72bc0d

fixed unused myID vars to be at self level

4aa8974

counter fix

4a3d6f5

marcomangano requested a review from a team as a code owner October 31, 2020 10:03

marcomangano requested review from SichengHe, Xiaosong2105 and ewu63 October 31, 2020 10:03

ewu63 reviewed Oct 31, 2020

View reviewed changes

addressed comments

63334fb

marcomangano added 2 commits October 31, 2020 16:41

reverted back to original complexify.py

b4d1c7a

added .flake8 file to ignore issues with complexify

a07978b

run black on complexify

856f8ad

ewu63 previously approved these changes Oct 31, 2020

View reviewed changes

ewu63 requested a review from eirikurj October 31, 2020 16:18

eirikurj requested changes Nov 2, 2020

View reviewed changes

addressed comments

c6e112c

marcomangano dismissed ewu63’s stale review via c6e112c November 2, 2020 11:48

marcomangano requested review from eirikurj and ewu63 November 2, 2020 11:48

eirikurj approved these changes Nov 2, 2020

View reviewed changes

ewu63 approved these changes Nov 2, 2020

View reviewed changes

ewu63 merged commit 3363284 into mdolab:master Nov 2, 2020

marcomangano deleted the format-update branch November 3, 2020 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format update #35

Format update #35

marcomangano commented Oct 31, 2020

ewu63 Oct 31, 2020

ewu63 Oct 31, 2020

ewu63 Oct 31, 2020

marcomangano Oct 31, 2020

ewu63 Oct 31, 2020

ewu63 Oct 31, 2020

marcomangano Oct 31, 2020

ewu63 commented Oct 31, 2020

marcomangano commented Oct 31, 2020

marcomangano commented Oct 31, 2020

ewu63 commented Oct 31, 2020

marcomangano commented Oct 31, 2020

marcomangano commented Oct 31, 2020

ewu63 commented Oct 31, 2020

ewu63 left a comment

eirikurj left a comment

eirikurj Nov 2, 2020

eirikurj Nov 2, 2020

eirikurj Nov 2, 2020

eirikurj left a comment •

edited

Loading

marcomangano commented Nov 2, 2020

ewu63 commented Nov 2, 2020

Format update #35

Format update #35

Conversation

marcomangano commented Oct 31, 2020

Purpose

Type of change

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewu63 commented Oct 31, 2020

marcomangano commented Oct 31, 2020

marcomangano commented Oct 31, 2020

ewu63 commented Oct 31, 2020

marcomangano commented Oct 31, 2020

marcomangano commented Oct 31, 2020

ewu63 commented Oct 31, 2020

ewu63 left a comment

Choose a reason for hiding this comment

eirikurj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirikurj left a comment • edited Loading

Choose a reason for hiding this comment

marcomangano commented Nov 2, 2020

ewu63 commented Nov 2, 2020

eirikurj left a comment •

edited

Loading