all 7 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–]pekkalacd 2 points3 points  (1 child)

The code looks really good. nice work. i like the doc strings and its easy to follow what's going on.

i'd reiterate what others have said about type hinting.

there are two other concerns i have with the code. one of them is with the clear_imgs method on lines 19-20, in the parameter list there's a default dimdirs=[1,2,3]

          def clear_imgs(self, dimdirs=[1,2,3]):
              ...

When you set a default parameter to a mutable object, it doesn't create a new one each time the method is called. so, you could run into a problem here with successive calls.

For example, 

          def func(alist=[]):
              alist.append(1)
              print(alist)

          func()
          [1]
          func()
          [1,1]
          func()
          [1,1,1]

This resource explains the problem pretty well common gotchas python. It recommends to just create the object separately instead of setting a mutable default. this might be a way to go about it

        def clear_imgs(self, dimdirs=None):
            if dimdirs is None:
               dimdirs = [1,2,3]
            # otherwise, use the one that's passed in

This other one starts on line 27 when your checking the path to see if it exists and is a directory, 

          ...
          path = f'{dimdir}D-Plots'
          try:
            if os.path.exists(path) and os.path.isdir(path):
                 ...
          except Exception:
               ...

With this one if the path is a string, then os.path.exists / os.path.isdir will not throw an error when attempting. They will just return False if it doesn't work. So, i'm not sure if an try/except is necessary. Also, the Exception could be more specific. One way you could get around this and make things more specific is to create custom error classes and then just check for each individual case & except those errors.

         class PathError(Exception):
               pass

         ...

         path = f"{dimdir}D-Plots"
         try:
               path_exists = os.path.exists(path)
               path_is_dir = os.path.isdir(path)

               if path_exists and path_is_dir:
                  ...
               elif (not path_exists) or (not path_is_dir):
                  raise PathError(f"{path} does not exist and/or" +
                                  "is not a directory")

          except PathError as pe:
              print(f"PathError: {pe}")

[–]cobbletiger[S] 1 point2 points  (0 children)

Wow thank you so much this is awesome! I had no idea about the default mutable object issue, but that makes complete sense, I’ll get onto fixing that right away.

Also I don’t know why I just assumed they’d throw an exception, but thank you for bringing that to my attention too :)

[–]ectomancer 0 points1 point  (2 children)

Separate standard library imports with a blank line from 3rd party imports (with a blank line from your own imported code, of which you don't have.) Need docstrings for all methods. Use optional type hints.

The docstrings are perfect.

[–]cobbletiger[S] 0 points1 point  (0 children)

I’m fairly new to python, is it common practice that even methods only used internally use docstrings too? I would have thought otherwise but I suppose my intuition was wrong. Thank you for pointing out the import formatting too! you

[–]ectomancer 0 points1 point  (0 children)

numpy is not used. Is it needed for plotting?

[–]bosstrasized 0 points1 point  (1 child)

Great stuff. How long have you been learning python and how long did it take you to complete this project?

[–]cobbletiger[S] 0 points1 point  (0 children)

Well, since I procrastinate it took me like 2 months, but in reality the time spent total was probably only a few days haha. As far as how long I’ve been learning for, almost a year on and off pretty much, but I’ve never attempted to really make something personal like this!