VideoSync Class Created #1
Conversation
|
Sorry for the delay, I'll try to find time early next week to review |
There was a problem hiding this comment.
I talked briefly with @NeuroLaunch today about the context for this PR. To summarize: the intent is a general-purpose tool that could be imported (and possibly have a CLI as well) and used to synchronize any arbitrary data streams (squid-OPM, squid-video, etc).
If that's right, then (1) this code should live in a proper python package (which this repo is not), and (2) the name should not be "duo-pilot". It might end up being easier to start a fresh repo using a package templating tool such as https://github.com/cookiecutter/cookiecutter (two others are "copier" and "cruft"). There's a helpful "cookie" template at https://github.com/scientific-python/cookie that is geared toward scientific python projects; that may be helpful or may be overkill for something this niche.
Aside from that high-level commentary: there's a lot of standard style stuff that isn't followed in this PR, I'll briefly mention them below just so you're aware of them; they're not blockers per se but rather habits worth cultivating that will save you time when contributing to other projects.
I'll let @NeuroLaunch comment on the API design, since I think he's got a better sense of what the use cases are for this tool and what the desirable outputs should be.
| import os | ||
| from scipy.io import wavfile | ||
| from scipy.io.wavfile import read as wavread | ||
| import mne |
There was a problem hiding this comment.
sort imports. Two tools are isort and ruff, and there are IDE plugins for both. Most projects I interact with are using ruff nowadays (which also does lots of other linting tasks, not just import sorting).
| class VideoSync: | ||
| def __init__(self, input_folder=None, output_folder="/output"): | ||
| """ | ||
| Summary: VideoSync represents an object performing synchronization between multiple streams of data. |
There was a problem hiding this comment.
there are standards around line length and the content and phrasing of docstrings that are worth knowing about. pydocstyle is the thing to search here, though again I think maybe ruff does these too?
| print("video_path has not yet been instantiated. Please use set_video_path or parse_files_from_folder to instantiate this attribute.") | ||
| return None |
There was a problem hiding this comment.
several cases like this below; I would have expected this to raise an error rather than return None, but maybe I'm not understanding / envisioning the usage clearly/correctly.
| """ | ||
| temp = [] | ||
| if type(paths) != list: | ||
| raise TypeError("Cannot set meg paths to given input. Expected list, recieved " + type(paths) + " Paths supplied must be contained within a list in format [s1, s2, ..., sn].") |
There was a problem hiding this comment.
use f-strings rather than joining strings with +
| self.meg_paths: List of RELATIVE paths to meg files. | ||
| """ | ||
| # User defined paths for input/output directories | ||
| self.__input_folder = input_folder |
There was a problem hiding this comment.
starting attributes/variable names with double-underscore is unusual. A leading single underscore for "private" functions/classes/attributes is common.
| video_paths = [] | ||
| meg_paths = [] | ||
| for file in files: | ||
| ext = os.path.splitext(file)[1] |
There was a problem hiding this comment.
consider using the pathlib.Path interface rather than os.path. It's generally considered "the modern replacement for os.path" so new code should probably use it unless there's a strong need to stick with os.path
VideoSync class created with basic functionality: