Nm database #350

SamedVossberg · 2024-06-26T12:52:30Z

Integration of nm_database (new module)
changes in run method -> can now receive flag "save_csv" to save full df as csv (otherwise only head)
adjusted examples (save csv in run methods as I think it makes sense for the examples if people can look into the full csv files)

py_neuromodulation/nm_database.py

py_neuromodulation/nm_stream_offline.py

… for db

timonmerk

Thanks for those changes @SamedVossberg. I have a few more code requests related to typehints in the function declarations, and making the number of samples / time interval between saves a function parameter.

py_neuromodulation/nm_database.py

py_neuromodulation/nm_stream.py

tests/test_all_features.py

timonmerk

@toni-neurosc Can you check if this PR is in line with your current GUI backend setup? It looks fine in my opinion

toni-neurosc · 2024-07-26T15:32:26Z

py_neuromodulation/nm_stream.py

                if self.verbose:
                    if last_time is not None:
                        logger.debug("%.3f seconds of new data processed", time_[-1] - last_time)
+                        feature_dict["time"] = time_[-1] - last_time
+                    else:
+                        feature_dict["time"] = 0
                    last_time = time_[-1]


I think the recording of the time is not quite right here. The logger should be outputting the seconds processed for the first batch, and the recorded time should be either the start or the end of the batch, not the length of it. I have attempted a fix in my commit, check it out.

But here the intention was to say how much data was processed in the new batch, since this can actually vary with LSL

Also, did you already push the commit?

Not yet, I'm doing a bunch of other changes. My point here is, there are 2 things you might want to calculate:

How much data you have process, in terms of time period: for this you of course take the difference between the last time of the previous batch, and the first of the current batch. You display it in the logger for sure, but you can also save it, but where?

The timestamp of each sample: Since the timestamp for each single data point gets lost during feature calculation (due to resampling, averaging, and other transformations) you would default to using either the first timestamp of the batch, the last, or maybe the average.

Now the question would be, which of these belongs in the features_df["time"] field? Back in the office I proposed having 2 separate columns, even 3 (an additional 1 for batch number or processing timestamp).

toni-neurosc · 2024-07-26T15:45:58Z

py_neuromodulation/nm_stream_abc.py

@@ -155,6 +156,9 @@ def save_after_stream(

        self.save_sidecar(out_path_root, folder_name)

+        if not save_csv:
+            feature_arr = feature_arr.head()


I think it's better to not load the whole dataframe and subset it here, we can read just the first line from the SQLite database and pass it to this function

toni-neurosc

So I have made a bunch of changes:

Got rid of the out_path_root and folder_name in all the internal functions, and handled everything at the Stream.run level.
Removed the cast_values function from the database class as I don't think it belongs there, I just inlined it in the stream.run method
I made some optimizations to the database after looking some articles on SQLite optimization, mainly to make sure we're going fast but also not losing data in case of crashes or system failures
Moved the create_table function inside insert_data and the corresponding flag to the database class to not have internal database state mixed up with the stream class
Added a head() method to the database class to read only 1 line from the database and avoid reading all the data to save just the header.
Changed the code relating to the timestamps, more details in the review comments

I'm fine when it comes to the GUI side, I will merge this into the gui branch and if required make changes there.

Btw, I made some tests fail somehow, will commit the fixes later.

I also have a big refactoring in the works that I almost finished when I was in Berlin, I'll try to finish that ASAP and make a PR because I don't want the GUI branch diverging too much from main either.

toni-neurosc · 2024-07-26T16:02:22Z

py_neuromodulation/nm_stream.py

+
+        self.save_after_stream(out_path_root, folder_name, feature_df, save_csv=save_csv)
+
+        db.close()

        return feature_df



I'm not sure if returning the dataframe makes a lot of sense anymore, since it's only holding the 1 row when save_csv = False. I know it's needed for some examples, but maybe for offline analysis or custom analysis we should provide a different interface.

Yes, but I think it's still nice interface to kind of see the structure what features were calculated for which channels

The problem is, for long recordings, the moment the recording finishes it will try to load the entire database in memory and if it's too big it will hand or crash. Luckily thanks to the db itself this wouldn't cause a data-loss, but it's kinda awkward. For now I have added a return_df parameter True by default.

toni-neurosc · 2024-07-26T16:10:49Z

py_neuromodulation/nm_database.py

+            self.csv_path = Path(csv_path)
+
+        if os.path.exists(self.db_path):
+            os.remove(self.db_path)


I think it's better to throw an error here than straigh-up deleting a previous file if it exists, this could lead to unwanted data losses

Very elegant solution with the incremental file names!

toni-neurosc · 2024-07-26T16:27:25Z

py_neuromodulation/nm_stream.py

                if self.verbose:
                    if last_time is not None:
                        logger.debug("%.3f seconds of new data processed", time_[-1] - last_time)
+                        feature_dict["time"] = time_[-1] - last_time
+                    else:
+                        feature_dict["time"] = 0
                    last_time = time_[-1]


Not yet, I'm doing a bunch of other changes. My point here is, there are 2 things you might want to calculate:

How much data you have process, in terms of time period: for this you of course take the difference between the last time of the previous batch, and the first of the current batch. You display it in the logger for sure, but you can also save it, but where?

The timestamp of each sample: Since the timestamp for each single data point gets lost during feature calculation (due to resampling, averaging, and other transformations) you would default to using either the first timestamp of the batch, the last, or maybe the average.

Now the question would be, which of these belongs in the features_df["time"] field? Back in the office I proposed having 2 separate columns, even 3 (an additional 1 for batch number or processing timestamp).

toni-neurosc · 2024-07-26T16:41:56Z

py_neuromodulation/nm_stream.py

+            if not data_acquired:
+                db.create_table(feature_dict)
+                data_acquired = True
+            db.insert_data(feature_dict)


I moved the table creation logic into the database class

toni-neurosc · 2024-07-26T16:46:20Z

py_neuromodulation/nm_stream.py


-            l_features.append(feature_dict)
+            feature_dict = db.cast_values(feature_dict)


I think that casting is not necessary, we should ensure that the feature outputs are always np.float64, and even if casting is necessary, I don't think the logic for casting should be inside of the database class

py_neuromodulation/nm_database.py

toni-neurosc · 2024-07-26T18:00:31Z

py_neuromodulation/nm_stream.py

@@ -134,23 +140,38 @@ def _run(
                data_batch.astype(np.float64)


I realized we don't need this, mne_lsl returns the data in np.float64 format and astype actually makes a whole copy of the data, which is not good for performance

toni-neurosc · 2024-07-26T18:02:32Z

py_neuromodulation/nm_stream.py

@@ -134,23 +140,38 @@ def _run(
                data_batch.astype(np.float64)
            )
            if is_stream_lsl:
-                feature_dict["time"] = time_[-1]
                if self.verbose:
                    if last_time is not None:
                        logger.debug("%.3f seconds of new data processed", time_[-1] - last_time)


I don't think DEBUG level messages should depend on verbose, since DEBUG usually is disabled by default unless you set the level of logging to DEBUG explicitely

toni-neurosc · 2024-07-26T18:07:22Z

py_neuromodulation/nm_stream.py


        while True:
            next_item = next(generator, None)

            if next_item is not None:
-                time_, data_batch = next_item
+                timestamps, data_batch = next_item


I changed this from time_ to timestamps, although I'm not sure is consistent with RawGenerator at this point

toni-neurosc · 2024-07-26T18:08:31Z

py_neuromodulation/nm_stream.py

+            feature_dict["time"] = (
+                batch_length if is_stream_lsl else np.ceil(this_batch_end * 1000 + 1)
+            )


I'm not entirely sure this is right for 2 reasons:

the "time" field should be consistent between file streams and lsl_streams, not having a duration for one and a time point for the other

Not sure if the units are consistent between both types of streams right now

- Replace hyphens with underscores for re-referenced channel names

SamedVossberg added 15 commits June 20, 2024 15:48

first version of database integration

c3e60c7

debug db commit

6f7d7e5

debug paths, adjust timing for lsl

5493b28

check permission of github workflow

1742b93

fixing permission

39d7a1e

fixing gh workflow permission

edcac19

set permissions for file and dir

348f398

checking for paths before setting permissions

4ba784a

fixing permissions

87edcc9

another try fixing gh workflow permissions

434168d

adding time_idx to db path to resolve conflicts

d073663

refactor the database

7e86324

possibility to save feature_df as csv

b181749

auto delete old example dbs

4964333

debug db handling

37813d4

timonmerk reviewed Jun 26, 2024

View reviewed changes

py_neuromodulation/nm_database.py Outdated Show resolved Hide resolved

timonmerk reviewed Jun 26, 2024

View reviewed changes

py_neuromodulation/nm_stream_offline.py Outdated Show resolved Hide resolved

SamedVossberg added 6 commits June 26, 2024 16:46

conditional create table in run method, delete permission adjustments…

f91cbcc

… for db

debug times

e586cfa

changed paths for dbs and db creation

d1c6ecd

clean up nm_database

16fa9d5

resolve nm_steam file name

a1bd366

Merge branch 'main' into nm_database

e315fca

timonmerk requested changes Jun 28, 2024

View reviewed changes

py_neuromodulation/nm_database.py Outdated Show resolved Hide resolved

py_neuromodulation/nm_database.py Outdated Show resolved Hide resolved

py_neuromodulation/nm_stream.py Outdated Show resolved Hide resolved

tests/test_all_features.py Show resolved Hide resolved

SamedVossberg added 2 commits June 28, 2024 14:18

added typehints, added save_interval param to run

aa3250a

add last commit to db

8aac64d

timonmerk requested a review from toni-neurosc July 4, 2024 12:24

timonmerk approved these changes Jul 4, 2024

View reviewed changes

toni-neurosc reviewed Jul 26, 2024

View reviewed changes

Toni review

f577d57

toni-neurosc reviewed Jul 26, 2024

View reviewed changes

toni-neurosc added 5 commits July 26, 2024 20:25

Fix db bug

7a1cf33

- Replace spaces with underscores for freq band

ccfcc88

- Replace hyphens with underscores for re-referenced channel names

Change "folder_name" parameter for "prefix" downstream of nm.Stream

1c181e5

Generate unique filenames for new database files

de64766

Format and fix Ruff warnings

f101788

toni-neurosc approved these changes Jul 29, 2024

View reviewed changes

timonmerk added 3 commits August 1, 2024 11:52

change default params

765a182

add simple test for nm_db write

d037a05

add catch to create test folder

e058b96

toni-neurosc force-pushed the nm_database branch from e38486a to a132fea Compare August 1, 2024 10:44

simplify nm_database create out_dir

c0d9ff1

toni-neurosc force-pushed the nm_database branch from 0403734 to c0d9ff1 Compare August 1, 2024 10:47

toni-neurosc merged commit 88d15aa into main Aug 1, 2024
6 checks passed

toni-neurosc deleted the nm_database branch August 29, 2024 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nm database #350

Nm database #350

SamedVossberg commented Jun 26, 2024

timonmerk left a comment

timonmerk left a comment

toni-neurosc Jul 26, 2024

timonmerk Jul 26, 2024

timonmerk Jul 26, 2024

toni-neurosc Jul 26, 2024

toni-neurosc Jul 26, 2024

toni-neurosc left a comment

toni-neurosc Jul 26, 2024

timonmerk Aug 1, 2024

toni-neurosc Aug 1, 2024

toni-neurosc Jul 26, 2024

timonmerk Aug 1, 2024

toni-neurosc Jul 26, 2024

toni-neurosc Jul 26, 2024

toni-neurosc Jul 26, 2024

timonmerk Aug 1, 2024

toni-neurosc Jul 26, 2024

toni-neurosc Jul 26, 2024

timonmerk Aug 1, 2024

toni-neurosc Jul 26, 2024

toni-neurosc Jul 26, 2024


		l_features.append(feature_dict)
		feature_dict = db.cast_values(feature_dict)

		@@ -134,23 +140,38 @@ def _run(
		data_batch.astype(np.float64)

Nm database #350

Nm database #350

Conversation

SamedVossberg commented Jun 26, 2024

timonmerk left a comment

Choose a reason for hiding this comment

timonmerk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toni-neurosc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment