Duplicated and unnecessary code

[jdmoorman]

There are large amounts of duplicated or unnecessary code throughout the codebase. Some particular examples are

• [x] The __init__() function can be omitted from the classes SPOD_streaming, SPOD_low_ram, and SPOD_low_storage since it just calls super().__init__() in all three cases.
• [x] The majority of spod_low_ram.py and spod_low_storage.py are the same with minor differences for data management. The two can be combined or the shared/equivalent portions can be abstracted into separate functions.
• [x] Unused imports and unused variables can be deleted.
• [x] The variables CWD, CF, CFD, and BYTE_TO_GB are defined in many files but only used in a few.
• [x] Lines 9-16 of __init__.py can likely be deleted
• [x] The following code blocks (or similar) occur repeatedly throughout postprocessing.py. Consider abstracting them into functions.

  • if xticks:
      ax.set_xticks(xticks)
      ax.set_xticklabels(xticks)
    if yticks:
      ax.set_yticks(yticks)
      ax.set_yticklabels(yticks)

  • if isinstance(vars_idx, int):
    vars_idx = [vars_idx]
    if not isinstance(vars_idx, (list,tuple)):
      raise TypeError('`vars_idx` must be a list or tuple')

  • # save or show plots
    if filename:
      if path == 'CWD': path = CWD
      plt.savefig(os.path.join(path,filename), dpi=200)
      plt.close()
    else:
      plt.show()

  • # overlay coastlines if required
    if coastlines.lower() == 'regular':
      coast = loadmat(os.path.join(CFD,'plotting_support','coast.mat'))
      real_ax.scatter(coast['coastlon'], coast['coastlat'],
                marker='.', c='k', s=1)
    elif coastlines.lower() == 'centred':
      coast = loadmat(os.path.join(CFD,'plotting_support','coast_centred.mat'))
      real_ax.scatter(coast['coastlon'], coast['coastlat'],
                        marker='.', c='k', s=1)

  • real_ax.set_xlim(np.nanmin(x1)*1.05,np.nanmax(x1)*1.05)
    real_ax.set_ylim(np.nanmin(x2)*1.05,np.nanmax(x2)*1.05)

  • real_ax.axhline(x1[idx_x1], xmin=0, xmax=1,color='k',linestyle='--')
    real_ax.axvline(x2[idx_x2], ymin=0, ymax=1,color='k',linestyle='--')

• [x] In postprocessing.py, you often do

  if filename:
      basename, ext = splitext(filename)

  but only use basename and ext later e.g.

  # save or show plots
  if filename:
      if path == 'CWD': path = CWD
      filename = '{0}_var{1}_mode{2}{3}'.format(basename, var_id, mode_id, ext)
      plt.savefig(os.path.join(path,filename),dpi=400)
      plt.close(fig)
  if not filename:
      plt.show()

  You can combine the former into the latter as

  # save or show plots
  if filename:
      if path == 'CWD': path = CWD
      basename, ext = splitext(filename)
      filename = '{0}_var{1}_mode{2}{3}'.format(basename, var_id, mode_id, ext)
      plt.savefig(os.path.join(path,filename),dpi=400)
      plt.close(fig)
  if not filename:
      plt.show()

• [x] There is no need to run all of the plotting functions in every test. Ideally, the plotting functions should be tested independently from functionality of the algorithms since plotting is very time consuming. I suggest getting rid of any plotting from the tests except for in test_postprocessing().
• [x] Many of the tests are currently modified versions of the tutorials. At least, test_basic.py is almost identical to methods_comparison.py. If the objective of such tests is to ensure the tutorials don't become broken, it is preferable to run the tutorial code directly rather than running a copy of the code in case the tutorials are modified later.
• [x] weights is imported in all of the tutorials and tests but rarely used.
• [x] In __init__.py, you can provide direct access to the classes SPOD_base, etc by changing

  __all__ = ['spod_base', 'spod_low_storage', 'spod_low_ram', 'spod_streaming']

  to

  __all__ = ['SPOD_base', 'SPOD_low_storage', 'SPOD_low_ram', 'SPOD_streaming']

  so that the imports

  from pyspod.spod_low_storage import SPOD_low_storage
  from pyspod.spod_low_ram     import SPOD_low_ram
  from pyspod.spod_streaming   import SPOD_streaming

  can be simplified to

  from pyspod import SPOD_low_storage, SPOD_low_ram, SPOD_streaming

  or just import pyspod if you don't mind typing e.g. pyspod.SPOD_low_storage rather than SPOD_low_storage.

[mengaldo]

@jdmoorman Thank you, very useful insights! I should have addressed the majority of them, except a few. Below some comments regarding the ones not fixed.

Q.) "The init() function can be omitted from the classes SPOD_streaming, SPOD_low_ram, and SPOD_low_storage since it just calls super().init() in all three cases."
A.) How do you suggest to modify the code here?

Q.) "The majority of spod_low_ram.py and spod_low_storage.py are the same with minor differences for data management. The two can be combined or the shared/equivalent portions can be abstracted into separate functions." A.) I would like to maintain readability of the two while accepting some code duplication. Do you have any specific suggestions?

Q.) "Lines 9-16 of init.py can likely be deleted." A.) I would keep them such that if a user wants to run the library from source this maintains paths consistency and inclusion.

Q.) "In init.py, you can provide direct access to the classes SPOD_base, etc by changing" A.) For some reasons I did not manage to make it work. I got a circular dependency error.

[jdmoorman]

Q.) "The init() function can be omitted from the classes SPOD_streaming, SPOD_low_ram, and SPOD_low_storage since it just calls super().init() in all three cases." A.) How do you suggest to modify the code here?

Get rid of the __init__() method of SPOD_streaming, SPOD_low_ram, and SPOD_low_storage. i.e. Delete

• [x] lines 24-25 of spod_streaming.py
• [x] lines 31-32 of spod_low_ram.py
• [x] lines 31-32 of spod_low_storage.py

Q.) "The majority of spod_low_ram.py and spod_low_storage.py are the same with minor differences for data management. The two can be combined or the shared/equivalent portions can be abstracted into separate functions." A.) I would like to maintain readability of the two while accepting some code duplication. Do you have any specific suggestions?

I suggest creating a class called e.g. SPOD_traditional_base which implements the parts of SPOD_low_ram and SPOD_low_storage that are duplicated, and have SPOD_low_ram and SPOD_low_storage inherit from SPOD_traditional_base. You can abstract any blocks of code that differ between SPOD_low_ram and SPOD_low_storage into virtual methods in SPOD_traditional_base which are overridden by methods in SPOD_low_ram and SPOD_low_storage.

Q.) "Lines 9-16 of init.py can likely be deleted." A.) I would keep them such that if a user wants to run the library from source this maintains paths consistency and inclusion.

This is not necessary for any user who follows your installation instructions from the readme. Do you work with such users? What is the benefit of running the library without installing it properly?

Q.) "In init.py, you can provide direct access to the classes SPOD_base, etc by changing" A.) For some reasons I did not manage to make it work. I got a circular dependency error.

I cannot reproduce your issue. Please provide the example you ran that resulted in such an error.

[mengaldo]

Q.) This is not necessary for any user who follows your installation instructions from the readme. Do you work with such users? What is the benefit of running the library without installing it properly? A.) I work with such users, but I am actively developing some additional functionalities and would prefer those lines. I do not think they create a critical issue within the library.

Q.) I suggest creating a class called e.g. SPOD_traditional_base which implements the parts of SPOD_low_ram and SPOD_low_storage that are duplicated, and have SPOD_low_ram and SPOD_low_storage inherit from SPOD_traditional_base. You can abstract any blocks of code that differ between SPOD_low_ram and SPOD_low_storage into virtual methods in SPOD_traditional_base which are overridden by methods in SPOD_low_ram and SPOD_low_storage.

A.) This will require some time.

Q.) I cannot reproduce your issue. Please provide the example you ran that resulted in such an error. A.) I changed the piece of code you suggested:

"""PySPOD init"""
# __all__ = ['spod_base', 'spod_low_storage', 'spod_low_ram', 'spod_streaming']
__all__ = ['SPOD_base', 'SPOD_low_storage', 'SPOD_low_ram', 'SPOD_streaming']

# from .spod_base        import SPOD_base
# from .spod_low_storage import SPOD_low_storage
# from .spod_low_ram     import SPOD_low_ram
# from .spod_streaming   import SPOD_streaming
from pyspod import SPOD_low_storage, SPOD_low_ram, SPOD_streaming

And here is what I get.

python test_basic.py 
Traceback (most recent call last):
  File "test_basic.py", line 15, in <module>
    from pyspod.spod_low_storage import SPOD_low_storage
  File "/Users/gian/GIT-GM/pyspod-review/tests/../pyspod/__init__.py", line 9, in <module>
    from pyspod import SPOD_low_storage, SPOD_low_ram, SPOD_streaming
ImportError: cannot import name 'SPOD_low_storage' from partially initialized module 'pyspod' (most likely due to a circular import) (/Users/gian/GIT-GM/pyspod-review/tests/../pyspod/__init__.py)

[mengaldo]

@jdmoorman Duplicated code shared by SPOD_low_ram and SPOD_low_storage has now been addressed. I added three common methods, compute_blocks, compute_standard_spod, and store_and_save, in SPOD_base. Thank you.