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

Writer wishlist #56

Open
3 tasks
tstenner opened this issue Nov 1, 2019 · 19 comments
Open
3 tasks

Writer wishlist #56

tstenner opened this issue Nov 1, 2019 · 19 comments
Labels
discussion Everyone's asked to put one's oar in

Comments

@tstenner
Copy link
Contributor

tstenner commented Nov 1, 2019

I've started a rough prototype for writer support. Currently, it looks like this:

with pyxdf.writer.Writer('/tmp/foo.xdf.gz') as w:
    w.add_stream(streamid=1, header='<info><streamname>XYZ</streamname>…</info>')
    w.add_stream_data(streamid=1, data=np.array([[1,2],[2,3],[3,4]], ts=np.array([5.1, np.nan, 5.3]))
    w.add_stream_offsets(streamid=1, offsets=np.array([[1.0, 5.0], [1.3, 5.3]]))

Goals:

  • Acceptable performance in pure Python
  • Optional cython-compiled parts
  • Full support for the XDF 1.0 spec

Non-goals:

  • accept the reader output directly as input
  • type conversion (i.e. convert data to the stream's data type; write str objects to a string stream)
  • write everything in C and call a compiled library
@tstenner tstenner added the discussion Everyone's asked to put one's oar in label Nov 1, 2019
@Argzero
Copy link

Argzero commented Dec 30, 2019

I have a rudimentary exporter nearly complete in pure python using numpy and such at the moment. Needs some adjustments for the XDF to be readable in sigviewer. I was also planning to add a few features not included in the LabRecorder / listed here. I will post here when I am done but I'm going to be delayed until after my MCAT in Mid-Jan. I'll be in touch.

After I get the python basic functionality up and running, I'll toss my code onto GitHub and post a link here. I also plan to re-implement it in C or C++ afterward.

@tstenner
Copy link
Contributor Author

I've also started a prototype, could you upload your not-yet-ready code so I could take a look at it?

@LMBooth
Copy link

LMBooth commented May 27, 2021

I don't suppose there's been any progress on this front from anyone? @tstenner @Argzero

@LMBooth
Copy link

LMBooth commented Jun 27, 2021

For anyone looking for a python wrapper for an xdf writer, @agricolab has done a great job with pyLieSL, following the example for the LabRecorderCLI here.

@pablomainar
Copy link

Hi, any progress on this? I'm looking for a way of loading an XDF, modifying it and saving it back. I have checked pyLieSL but it doesn't seem to be able to do that (correct me if I'm wonrg).

@agricolab
Copy link
Member

No, it can't.

@s-de-haan
Copy link

Would be great to have this feature

@cbrnr
Copy link
Contributor

cbrnr commented Jun 2, 2022

@rob-luke, you mentioned elsewhere that you (or someone from your group) might be interested in contributing. Is this still true? Do you want to discuss your ideas here? I don't think that anyone else currently has the resources to implement a writer.

@rob-luke
Copy link

Hi @cbrnr,

Indeed, we have written an XDF writer for our needs at our workplace. Sorry for the delay in responding to you, I have taken some time to look at both code bases and look at potential barriers to submitting the code here. I will describe the code we have and the potential barriers to merging. Then we can decide how to proceed together.

  • pxydf only requires numpy. Our code requires pydantic. Would you be open to adding pydantic as a requirement? (I understand that adding requirements is a big decision, so let's decide this up front)
  • Our writer is not a streaming writer as described in this issue. It simply writes an entire file in one shot. Would this be acceptable?

Thanks for considering these issues. It would be great to clarify these issues up front. The next step would be to decide on an API, then I could open a PR. Thanks 🙏

@cbrnr
Copy link
Contributor

cbrnr commented Jun 20, 2022

Regarding the second point, I don't think we intended to implement a streaming writer. This is already done by LSL, and the purpose of an XDF writer here would be to dump data (which is already available in some form TBD) into XDF.

Re pydantic, would it be possible to remove the type checks? Or does your writer critically rely on this functionality so it doesn't work without it?

@rob-luke
Copy link

Re pydantic, would it be possible to remove the type checks? Or does your writer critically rely on this functionality so it doesn't work without it?

Unfortunately, it critically relies on this and won't work without it. My primary concern here is that this will change the versions of python that pyxdf would work with. Plus the additional changes to maintenance etc.

@cbrnr
Copy link
Contributor

cbrnr commented Jun 20, 2022

This would require Python 3.7+, which is fine with me. I've never worked with pydantic, but I assume it could be replaced by data classes (and removing all type checks) if we really don't want it?

@rob-luke
Copy link

I assume it could be replaced by data classes (and removing all type checks) if we really don't want it?

Correct. Pydantic handles lots of checking of types and conversion too, to ensure you pass in the right data. So this would need to be all written manually instead.

@cbrnr
Copy link
Contributor

cbrnr commented Jun 20, 2022

OK, so it could be done, and the least effort solution would be to not check and convert at all, but assume that the input is in the desired format.

Let's wait what others have to say though. @cboulay @tstenner @agricolab

@agricolab
Copy link
Member

agricolab commented Jun 23, 2022

Thanks for the contribtution, @rob-luke!

My five cents: Although i personally am a fan of type hints and their benefits for static code analysis and readability, their enforcement during runtime is not pythonic, and prevents a lot of things i love in python (like easy monkey patching). I would prefer to drop type checking.

As i understand it, you use the type for control flow, don't you? If the type of an argument is relevant for control flow, overloading could be alternative solution which is easy to extend, and would even work for init: E.g. https://github.com/dabeaz/python-cookbook/blob/master/src/9/multiple_dispatch_with_function_annotations/example1.py#L94-L121

@cboulay
Copy link
Contributor

cboulay commented Jun 27, 2022

FYI, LabRecorder was originally a Python app so you can find some inspiration in that old code.
https://code.google.com/archive/p/labstreaminglayer/source/default/source

@rob-luke
Copy link

Thank you @cbrnr, @agricolab, and @cboulay for your considered responses.

From the feedback above I understand that adding pydantic as a dependency is not ideal, I understand the reasons for this. Unfortunately with our current workload, we can not commit to rewriting our xdf writer to remove pydantic. Thanks for your consideration. We will keep this on our internal project list, and if our development capacity increases we can revisit this in the future.

Thanks for maintaining such a great project, we use it regularly and hope to contribute back when we can.

@LMBooth
Copy link

LMBooth commented Mar 6, 2023

Thank you @cbrnr, @agricolab, and @cboulay for your considered responses.

From the feedback above I understand that adding pydantic as a dependency is not ideal, I understand the reasons for this. Unfortunately with our current workload, we can not commit to rewriting our xdf writer to remove pydantic. Thanks for your consideration. We will keep this on our internal project list, and if our development capacity increases we can revisit this in the future.

Thanks for maintaining such a great project, we use it regularly and hope to contribute back when we can.

Hi @rob-luke , is it possible to find a link to your code anywhere which uses pydantic for other to use in the mean time?

@GiannisKat123
Copy link

I have a writer in the works that creates .xdf files that is equivalent to the implementation of the LabRecorder's XDFwriter. Currently, it:

    1. Written for normal lists or numpy arrays.
    1. Thread-safe, for multi-streaming (assuming that headers and footers are written at the start and end of the writing).

Cython could be included. Should I request a PR when it's done?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Everyone's asked to put one's oar in
Projects
None yet
Development

No branches or pull requests

10 participants