Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design strategy #8

Open
tsalo opened this issue Jul 10, 2020 · 0 comments
Open

Design strategy #8

tsalo opened this issue Jul 10, 2020 · 0 comments
Labels
Discussion Discussion of a concept or implementation. Need to stay always open.

Comments

@tsalo
Copy link
Member

tsalo commented Jul 10, 2020

In our monthly developers call, we discussed how we'd like phys2denoise to be structured (along with the other packages). Here are the minutes for anyone who's interested.

There were a few ideas that were discussed. Some of these ideas have broad implications for the whole suite, and, if we decide to go with one of those, then we can figure out how to synchronize package-specific discussions within the suite.

  1. phys2denoise operates on a shared object with peakdet. This class would represent physio data from a single frequency, and could end up being restricted to a single channel.
    • Pros:
      1. Classes group attributes (e.g., the time series and the sampling rate) together.
      2. peakdet will need its own class to track changes to the data. While we don't need that same functionality in phys2denoise, it's helpful to have the same design philosophy across packages.
    • Cons:
      1. We would need to either (1) duplicate our class definition across peakdet and phys2denoise or (2) define the class in one package and then manage changes to the class across both.
      2. For "higher level" users, who want to use the individual functions, having to initialize or operate on a new object for their data (instead of a common one like a numpy array or a pandas DataFrame) will make it harder for them to start using the package.
      3. Mocking up package-specific objects for tests is hard and those objects can add overhead to your CI.
  2. phys2denoise's object is moved all the way up to physutils. Same pros and cons as the above, but with additional difficulties in synchronizing the class definition across packages.
  3. A purely functional approach. All functions in phys2denoise would operate on arrays or possibly DataFrames, with no package-specific classes defined.
    • Pros:
      1. This makes the individual functions easier to work with.
      2. Most users will only interact with the main workflow function/CLI, so the internal structure of the function is not important (i.e., classes are not necessary).
      3. Functions are easier to test.
    • Cons:
      1. Devs who get used to OOP in peakdet and phys2bids might be confused by phys2denoise, although the opposite is more likely, since functional programming is more intuitive.

If anyone has other ideas, please comment and I can update this issue summary.

@smoia smoia added the Discussion Discussion of a concept or implementation. Need to stay always open. label Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Discussion of a concept or implementation. Need to stay always open.
Projects
None yet
Development

No branches or pull requests

2 participants