-
Notifications
You must be signed in to change notification settings - Fork 34
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
#175674845 ; module for raw data format #160
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions, but I like the approach!
@@ -199,7 +198,17 @@ def load_signal_model(model_class: SignalModel, | |||
return signal_model, filename | |||
|
|||
|
|||
def load_csv_data(filename: str = None) -> str: | |||
def choose_csv_file(filename: str = None) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
TIMESTAMP_COLUMN = 'timestamp' | ||
|
||
|
||
class RawData: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We spoke about this in person briefly! I think this could reside in acquisition? I don't foresee using this structure outside that module. If you can think of one, we should leave it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RawData object is now used in a number of different places including offline_analysis. I think it makes sense to have it at a higher level than acquisition since it's a cross cutting concern. I do think it would make sense to introduce a core
module for things like this, though. It's not really a helper.
self._csv_writer.writerow(row) | ||
|
||
|
||
def load(filename: str) -> RawData: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that we can support both full read and iterative. It makes a lot of sense
bcipy/helpers/tests/test_raw_data.py
Outdated
def test_write_with_no_data(self): | ||
"""Test that raw data can be persisted to disk.""" | ||
path = Path(self.temp_dir, 'test_raw_data.csv') | ||
data = RawData(daq_type='DSI-24', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some repeated steps in many of these unittests. It may be more change resilient to bring path/data up to self
and reconstruct them when required (using the LSL daq type, etc)
bcipy/helpers/tests/test_triggers.py
Outdated
'c3']) -> Tuple[str, List[float]]: | ||
"""Helper function for creating mock data that looks like the raw_data.csv | ||
output. Adds trigger data to the TRG column at the specified interval. | ||
def write_sample_raw_data(raw_data_path: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
write_sample_trigger_data? We can reserve RawData as a concept for acquisition data output.
Overview
Many BciPy modules depend on our raw_data.csv format to be structured in a specific way, and the code that writes and reads this data format is repeated in several places throughout the codebase. This PR creates a raw_data module that provides an interface for reading and writing the raw data format without knowing the details of its implementation.
Ticket
https://www.pivotaltracker.com/story/show/175674845
Contributions
Test