4
\$\begingroup\$

A bit of background:

Most often I am analysing data (mostly timeseries) of mechanical machine conditions (vibrations, temperatures...) that were acquired in different 'runs'. All runs were acquired over distinct non-overlapping periods in time. While they were all acquired in a common testing campaign with a certain sample rate, using a fixed set of sensors, etc. the runs all have individual (metadata) characteristics (e.g. the time of acquisition, the type of event that was recorded, a certain machine setting during acquisition...). Since I require similar operations for each of the runs (loading data, performing preprocessing, plotting the data...), my current approach is the following:

I define a TestCampaign class, for which I can define all shared parameters for all of the runs. Examples of methods for a TestCampaign instance are:

  • __init__ (of course) Where I can specify the path to the folder containing all data and the path to an excel file which holds all metadata for the campaign itself and all separate runs.
  • load_metadata Which loads the metadata excel file to class attribute(s). Each of the runs have a certain ID assigned to them in this metadata file in order to easily reference to them.
  • load_run_by_id Which creates a TestRun instance. To create this instance, only the ID of the run (as specified in the metadata excel file) is required and all specific information of the run is passed to this instance from the metadata attribute of the test campaign.

I also define a TestRun class. This has methods like:

  • __init__ Which takes the "parent" TestCampaign instance as input and stores it as an attribute for later reference. Additionally it takes all the metadata information for this run as input and stores that as an attribute as well.
  • load_data Which loads the data from a file for which the path is specified in the metadata

My current problem:

While I was writing code for my most recent project, I started wondering whether this 'standard' approach I take is actually Pythonic and whether I am unaware of some much more readable or efficient way of achieving what I want. I am aware that generic best practices are outside the scope of this site, so let's forget about this 'general structure' and focus specifically on my current project: Within a certain testcampaign, I recorded data for 3 different runs, for which I stored the information in a metadata xlsx-file (in tab 'Runlist'):

ID Date Time Folder Fault
1 2023-08-14 11:20:21 20230816 No fault
2 2023-08-15 09:45:03 20230816 Gear fault
3 2023-08-16 14:55:47 20230816 Bearing fault

For each run, I recorded thermal images with 8 sensors. Each sensor is placed on one side of the machine (Left or Right), a certain position (Outer or Inner) and the sensor is inspecting a certain machine component (Bearing or Gear), which is also included in the metadata xlsx-file (in tab 'Sensors'):

ID Full serial name Side Component Location
1-10056002a 00560055-...-203400000000 Right Gear Outer
1-00350020 00350020-...-203400000000 Right Bearing Outer
1-0040001f 0040001f-...-203400000000 Right Bearing Inner
1-00560055 0056002a-...-203400000000 Right Gear Inner
2-0053004e 0053004e-...-203400000000 Left Bearing Inner
2-0044002a 0044002a-...-203400000000 Left Gear Outer
2-00540025 00540025-...-203400000000 Left Gear Inner
2-00310021 00310021-...-203400000000 Left Bearing Outer

Each sensor generates a single numpy array of dimensions (120, 160), which are stored to a .npy-file. I would like to be able to load all data for each run individually and perform some operation on the data, e.g., plot the data based on their physical location for easy visual inspection, such as I implemented here:

import pandas as pd
import os
import matplotlib.pyplot as plt
from glob import glob

class TestCampaign():
    def __init__(self, datafolder_path='', metadata_filepath='metadata.xlsx'):
        self.datafolder_path = datafolder_path
        self.runlist, self.sensor_config = self.load_metadata(metadata_filepath)

    def load_metadata(self, filepath):
        excel_file = pd.ExcelFile(filepath)

        runlist = excel_file.parse('Runlist', index_col='ID')
        sensor_config = excel_file.parse('Sensors', index_col='ID')

        return runlist, sensor_config

    def load_run_by_id(self, run_id):
        return TestRun(self, run_id)
    
    def get_sensor_ids_for_setting(self, side=None, location=None, component=None):
        if side is None:
            side = self.sensor_config['Side'].unique()
        elif isinstance(side, str):
            side = [side]
        if location is None:
            location = self.sensor_config['Location'].unique()
        elif isinstance(location, str):
            location = [location]
        if component is None:
            component = self.sensor_config['Component'].unique()
        elif isinstance(component, str):
            component = [component]
        return self.sensor_config.index[self.sensor_config['Side'].isin(side)
                                        & self.sensor_config['Location'].isin(location) 
                                        & self.sensor_config['Component'].isin(component)].values    

class TestRun():
    def __init__(self, test_instance=TestCampaign(), run_id=1):
        self.test = test_instance
        self.info = self.test.runlist.loc[run_id]
        
        self.data = self.load_all_data()

    def load_all_data(self):
        # Load data of all sensors to single list
        data = {}
        for sensor_id in self.test.sensor_config.index.values:
            data[sensor_id] = self.load_data_sensor(sensor_id)
        return data

    def load_data_sensor(self, sensor_id):
        filenames = glob(os.path.join(self.test.datafolder_path,
                                        str(self.info.Folder),
                                        sensor_id,
                                        '*.npy'))
        if filenames:
            return np.load(filenames[0]) # only load the first file if there are multiple, this loads a numpy array of size (120, 160).
        else:
            print('No data found for', sensor_id)
            return []
    
    def plot_data(self, figsize=(12.8, 5)):
        if figsize == 'small':
            figsize = (6.4, 3)
        elif figsize == 'large':
            figsize = (12.8, 5)
        fig = plt.figure(constrained_layout=True, figsize=figsize)
        components = ['Gear', 'Bearing']
        
        # Divide figure in two subfigures for the two sides of the machine
        subfigs = fig.subfigures(nrows=1, ncols=2)
        for side, subfig in zip(['Left', 'Right'], subfigs):
            if side == 'Left':
                locations = ['Outer', 'Inner']
            else:
                locations = ['Inner', 'Outer']
            
            # Plot the images
            axes = subfig.subplots(nrows=2, ncols=2)
            for ax_r, location in zip(axes, locations):
                for ax_rc, component in zip(ax_r, components):
                    id = self.test.get_sensor_ids_for_setting(side=side, 
                                                              location=location, 
                                                              component=component)[0]
                    if np.size(self.data[id]) != 0:
                        ax_rc.imshow(self.data[id], label=id)
                    else: # deal with empty data arrays
                        ax_rc.imshow(np.zeros((120, 160)))
                    ax_rc.set_xticks([])
                    ax_rc.set_yticks([])

            # Lay-out
            subfig.suptitle(side + ' side')
            for ax, location in zip(axes[0], locations):
                ax.set_title(location)
            for ax, component in zip(axes[:,0], components):
                ax.set_ylabel(component, size='large')
        
        plt.show()

if __name__ == "__main__":
    
    datafolder = r"Sample data"
    metadata_filepath = r"Sample data/metadata.xlsx"

    tc = TestCampaign(datafolder, metadata_filepath)

    for run_id in range(3):
        tr = tc.load_run_by_id(run_id)
        tr.plot_data()

The key idea for my two classes is thus that I want to have general information at the TestCampaign level and run-specific information at the TestRun level. I use additional methods at both levels (e.g., for normalising data at run-level, or for comparing runs at test campaign-level), which I left out for simplicity. I feel like I am abusing the idea of classes here and my code is not really readable because of 'semi-nesting' one class into the other. Especially setting the default TestCampaign instance as the default value for test_instance during the initialisation of TestRun seems bad practice, as this can give problems with the folderpaths required for TestCampaign. I do this now anyways, to make sure my IDE can autofill the attributes and methods for self.test within the TestRun class. Ideas on how to improve the readability and fix this initialisation problem while achieving the same functionality would be highly appreciated.

EDIT: fixed the lay-out of the tables

EDIT2: clarified the initialisation problem of the TestRun class

\$\endgroup\$
1
  • \$\begingroup\$ "generic best practices are outside the scope" means questions solely pertaining to GBP -- 'which is betterer list or iter?' Asking 'how do I make my code betterer, even in respect to best practices' is ok. \$\endgroup\$
    – Peilonrayz
    Commented Sep 7, 2023 at 13:49

1 Answer 1

3
\$\begingroup\$

In the runlist sheet I confess I'm not excited to see each timestamp split into a (date, time) pair, but fine, whatever. It's ISO-8601 so we can't go too far wrong.

key idea [is] I want to have general information at the TestCampaign level and run-specific information at the TestRun level.

Sounds perfect.

abusing the idea of classes here [?]

No, not at all. It fits in nicely with normalization and with DRY. There should be one piece of code that knows how to do each required task, and there should be one piece of data (a class instance) that stores what we're required to know. The idea of a class is "data + behaviors", and you have the right chunk of data with the right behaviors attached to it.

code is not really readable because of 'semi-nesting' one class into the other

Not at all. Where you say "nesting", I say "abstraction". You have abstracted away the common setup from each test run. That lets a TestRun instance focus on what it should know about -- observations peculiar to that specific run.

my IDE can autofill the attributes and methods

Nothing to worry about. Easing the development workflow in that way sounds like a "requirement", which you've addressed in a perfectly natural way.


private method

    def load_metadata(self, filepath):

This is part of the __init__ ctor, and it's very nice that you broke it out in that way. For one thing, now it is unit testable on its own.

It is not part of your Public API, so an _ underscore prefix, _load_metadata, would be appropriate.

Similarly for _load_all_data down in TestRun. And very likely for _load_data_sensor.


each method offers some level of abstraction

We begin working down at the raw bits and bytes level, and gradually build up verbs that get us closer and closer to the business domain. Eventually at the end we invoke doit() or main(), which calls a handful of verbs at lower abstraction level, which make deeper calls until we're down at the machine level.

    def load_run_by_id(self, run_id):
        return TestRun(self, run_id)

This method does not appear to be pulling its weight. It doesn't abstract away any details.

If you feel you need a synonym with this spelling, at least put it where it belongs, over in the TestRun class. But I see little point in that.


default args

        if side is None:
            side = self.sensor_config['Side'].unique()

In get_sensor_ids_for_setting we could take advantage of a standard idiom for defaulting input args. Prefer:

        side = side or self.sensor_config['Side'].unique()

If caller specified side, then side = side is a no-op. If not, then None or foo always evaluates to just foo, and we assign that.

Similarly for location and component.

I'm not super excited about the "turn scalar foo into a list [foo]", as it tends to invite caller confusion and bugs. My preference is to insist that caller specified the Right Thing; that way library and caller are in sync, they have a common view of what's happening, with no automagical surprises. If you feel you need to accommodate both types of input, at least spell out the details in the method's """docstring""".

Concretely, consider putting ..., side: Optional[str] = None, ... in the signature, and then unconditionally use [side] to turn it into a list. That implies adding

from typing import Optional

A default of None is bog standard and perfectly nice. Given that there's no valid use case for "" empty string here, I feel ..., side: str = "", ... would be a bit tidier.


mutable default param

class TestRun():
    def __init__(self, test_instance=TestCampaign(), run_id=1):

Uggh! This is, kind of terrible. There's a whole class of bugs you really don't want to venture any place near to.

In C we distinguish between computing a quantity at "compile time" versus at "run time". In python the distinction is "import time" versus "run time". A mutable object was created when import evaluated the def, and creating several instances won't create several such objects. We just stick with the one from the import. Even if you only plan to create a single object, don't leave such a ticking time bomb lying around for future maintainers. Avoid the "mutable default" anti-pattern.

Standard idiom would be to put =None in the signature, and then:

        self.test = test_instance or TestCampaign()

... setting the default TestCampaign instance as the default value for test_instance during the initialisation of TestRun seems bad practice,

Consider not offering a default value at all, if you want caller to always explicitly specify it.


vague identifier

        self.info = self.test.runlist.loc[run_id]

Ok fine, maybe you really need that shorthand notation and you didn't want to use a @property decorator.

Calling it info doesn't help anyone. It looks like it's actually a run_id -- seems like a good identifier to use here.


comments lie!

    def load_all_data(self):
        # Load data of all sensors to single list
        data = {}

zomg, what is going on here? We promise the caller we'll load a list, and then in the next breath ignore that and start building a dict?

The code is specific, it gives the "how". Comments should be poetic (that is, vague, broad strokes), to give the "why". Call it a "container", if you like. Or rename the method to load_all_sensor_data and elide the comment.

    return np.load(filenames[0]) # only load the first file if there are multiple,
                                 # this loads a numpy array of size (120, 160).

Thank you for the helpful comment. It is fine, as far as it goes. Maybe I believe it. I bet it was true at one point, for at least one input file.

Consider rephrasing like this:

    arr = np.load(filenames[0])
    assert (120, 160) == arr.shape, arr.shape
    return arr

Now the Gentle Reader is well and truly convinced that every time this method will definitely be returning an array with exactly that shape. No need to read an English sentence which may or may not be true today. Or tomorrow.

        else:
            print('No data found for', sensor_id)
            return []

Consider raise ValueError(f'No data found for {sensor_id}'). I'm concerned that caller might view empty-list as a small amount of valid data.


use Path

Also, DRY.

    datafolder = r"Sample data"
    metadata_filepath = r"Sample data/metadata.xlsx"

I'm slightly sad that didn't come out as:

    from pathlib import Path

    datafolder = Path(r"Sample data")
    metadata_filepath = datafolder / "metadata.xlsx"

This codebase appears to achieve most of its design goals.

I would be willing to delegate or accept maintenance tasks on it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.