physiopy / physutils

Common tools for the physiopy suite
Apache License 2.0
0 stars 4 forks source link

Migrate Physio class from peakdet #2

Closed maestroque closed 4 months ago

maestroque commented 5 months ago

Migrating the Physio class from peakdet to physutils, aiming to be used in the general physiopy toolset

For starters, this PR only aims to provide a Physio class to be used by peakdet and phys2denoise.

Changes

I will keep this description updated for further changes

me-pic commented 5 months ago

@maestroque Good job on that PR ! You will probably notice /have probably already notice, but the current tests are failing because of a phys2bids import in test_physio_obj.py. However phys2bids is not included as a installation requirement

maestroque commented 5 months ago

@me-pic I was thinking to remove this file (physio_obj.py) for the time being. Since it was only here as a placeholder from what @smoia had mentioned. In the end, the integration of physutils for phys2bids is not something we are currently interested in for the moment What do you think?

maestroque commented 5 months ago

I deleted it for the time being, I think #1 covers us for the phys2bids object addition in the future, after the basic pipeline is down. Some features I'll add to the Physio object for the time being will be:

Then for the phys2bids object integration, we'll need to see if it can be done with a list of Physio objects. However, we'll need to figure out

m-miedema commented 5 months ago

I deleted it for the time being, I think #1 covers us for the phys2bids object addition in the future, after the basic pipeline is down. Some features I'll add to the Physio object for the time being will be:

  • Labels
  • Physio object type
  • Option to generate dummy Physio objects from peaks

Then for the phys2bids object integration, we'll need to see if it can be done with a list of Physio objects. However, we'll need to figure out

  • If they will have a common history file apart from the individual history, and how that could play out
  • How signals like TR will be treated, which do not need to be Physio objects

Sounds great! The labels will be to add BIDS complementarity i.e. subject and session info, if I'm interpreting this correctly from what we discussed earlier.

maestroque commented 5 months ago

I modified the history such that it will display the full function in the module

image

This way we will be able to integrate phys2denoise functions in the history (or physioqc or more). However the supported modules shall all be included inside load_history()

smoia commented 5 months ago

I think we should bring it forward, but with local imports in the neeed functions so that NK stays an optional dependency

maestroque commented 5 months ago

I believe this should be ready as well now. The only thing that remains is to add the automations for publishing. I'll add the yaml files and then @smoia adds the PyPI credentials etc. as GH secrets whenever he is available

@m-miedema @me-pic

maestroque commented 4 months ago

@me-pic @m-miedema I'm also working on the BIDS support for creating Physio objects (not pushed yet), should it be in this PR or merge this and open another?

me-pic commented 4 months ago

@maestroque I think we could merge this one and open another PR to integrate BIDS support !

maestroque commented 4 months ago

Ok then, merging