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

[WID-69] Support for EDF File Data in TSWidget #71

Merged
merged 5 commits into from
Apr 17, 2024

Conversation

peeplika
Copy link
Contributor

No description provided.

@peeplika
Copy link
Contributor Author

This PR addresses WID-69 by adding support for adding data to TSWidget in form of EDF files.
While the issue mentioned writing a WrapperEDF class, I found that we can retrieve data from EDF files directly as NumPy arrays, making it compatible with WrapperNumpy. However, I acknowledge that there may be aspects I've overlooked or that this approach may not align with the project's coding style.
Any feedback is appreciated so that I can proceed accordingly. Thank you!

@liadomide
Copy link
Member

@peeplika I think @romina1601 can have a better judge on the project coding style.

To me, this PR looks decent, but do you think it is possible to also write a unit-test with the new EDF related code ?
Also: can you tell us where did you find the test EDF file? is its license compatible for copying it here ?

@romina1601 romina1601 assigned peeplika and unassigned romina1601 Apr 15, 2024
@romina1601
Copy link
Member

Hi @peeplika ,
The code looks good, however I think it would be better to create a separate Wrapper for the EDF file (maybe one that extends WrapperNumpy?), just to keep it consistent with what we have so far.

@peeplika
Copy link
Contributor Author

Hello @liadomide @romina1601 Thanks for the feedback.
I have now created a seperate Wrapper for edf data and also added a test for it.
I took the test file from here -https://github.com/beacon-biosignals/EDF.jl/tree/main/test/data

@romina1601
Copy link
Member

Hi @peeplika ,
Thank you for your work on this issue. After discussing with @liadomide , we realized that if we allow the user to specify the path of a file as data argument for the plot_timeseries method, this would not be consistent with how our widgets work, because our widgets usually accept an in-memory object.
For this reason, I will edit this PR to merge it into another development branch from our repo (so not directly into main branch), I will make some changes to your code and then finally merge it into main. In this way, all your commits will be present into our repo and I can implement the necessary changes. 😊

@romina1601 romina1601 changed the base branch from main to WID-69 April 17, 2024 12:06
@romina1601 romina1601 merged commit a693a12 into the-virtual-brain:WID-69 Apr 17, 2024
0 of 3 checks passed
@peeplika
Copy link
Contributor Author

@romina1601 Sounds good!
Thanks for your consideration.
Please let me know if there is anything that I can assist with .

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.

3 participants