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

Implement #87: TSDFRecording based API #90

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Implement #87: TSDFRecording based API #90

wants to merge 6 commits into from

Conversation

twanvl
Copy link
Contributor

@twanvl twanvl commented Oct 23, 2024

This PR implements a more convenient interface for working with single-recording tsdf files.

See #87 for a detailed description and motivation.

The most basic use case now becomes:

df = read_dataframe(input_file)
write_dataframe(output_file, df)

@twanvl
Copy link
Contributor Author

twanvl commented Oct 23, 2024

The new functions should probably also be exported from tsdf/__init__.py

@twanvl
Copy link
Contributor Author

twanvl commented Oct 23, 2024

This also warrants an increase in the package version number.

@Erikpostt
Copy link
Contributor

Hey @twanvl, overall this looks very neat -- I'd like to merge this soon. I do have a few questions:

  • What is the reason you suggest to use "recordings"? Wouldn't it be consistent to use "time_series"?
  • You suggest using read_single_recording_metadata, read_multi_recording_metadata, and read_single_recording_dataframe. What do you think about combining the first two into read_recording_metadata, where: if len(recordings) ==1: return recordings[0] / elif len(recordings) > 1: return recordings / else: raise Exception()
  • If I load a dataframe using tsdf.read_single_recording_dataframe, and specify the channels parameter to a subset, it might be nice to automatically change the channels parameter of the .attrs attribute of the dataframe. Otherwise, I get AssertionError: The channel groups do not match the columns in the DataFrame when using tsdf.write_dataframe().
  • Perhaps you could add the functions the user wants to use (I think the reading/writing functions) to the __init__.py.

Let me know what you think. We could also merge your suggestions, and add issues related to my questions if you think they are valid.

@Erikpostt Erikpostt self-requested a review December 5, 2024 11:54
@twanvl
Copy link
Contributor Author

twanvl commented Dec 6, 2024

  • The name "recording" was used in some tsdf documentation I believe. If can also see the argument for TimeSeries or Series, and I am fine with changing it.
  • That is what read_single_recording_metadata does. But there can be meta data files that contain multiple recordings/time series.
  • to automatically change the channels when loading a subset: Yes, that makes sense.
  • Adding to __init__.py: Yes that should be done.

@Erikpostt
Copy link
Contributor

@twanvl you want me to implement these suggestions to your PR?

@twanvl
Copy link
Contributor Author

twanvl commented Dec 13, 2024

I have fixed all the issues mentioned above.

I also added an add_channel function to TimeSeriesMetadata, to make it easy to actually add channels and not just remove them. See the tests for an example.

I also added tests to ensure that all non-standard metadata is preserved when reading/writing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants