Skip to content

Edited read_xdf()#19

Open
lgarbar wants to merge 1 commit intocbrnr:mainfrom
lgarbar:fix-read_xdf
Open

Edited read_xdf()#19
lgarbar wants to merge 1 commit intocbrnr:mainfrom
lgarbar:fix-read_xdf

Conversation

@lgarbar
Copy link

@lgarbar lgarbar commented Nov 11, 2025

The current read_xdf() implementation in this repo fails for some XDF files that I have, likely due to differences in chunk structures or optional timestamps. I’ve made modifications to read_xdf() to:
• Gracefully handle all chunk types without failing on compressed or unusual files.
• Properly pre-allocate and populate arrays for both numeric and string streams.

These changes allow read_xdf() to successfully read all my XDF files while maintaining compatibility with the expected data structures. No other functionality in the repo was altered.

@cbrnr
Copy link
Owner

cbrnr commented Nov 14, 2025

Thank you for your contribution! However, I think you are mixing the necessary compatibility fix with several unrelated (and potentially breaking) changes.

The actual fix seems to be string handling for multi-channel data (correct me if I'm wrong, but the related issue should be #10):

if dtype === String
    for ch in 1:nchannels
        nbytes = Int(read_varlen_int(io))
        streams[id]["data"][curr_idx, ch] = String(read(io, nbytes))
    end

The other changes seem unrelated to me, so I suggest reverting them:

  1. The function docstring should contain the signature (see https://docs.julialang.org/en/v1/manual/documentation/#Functions-and-Methods), please do not remove it.
  2. The Julia logging system already provides functionality for debug and info messages, please do not work against it by introducing a debug parameter (see https://docs.julialang.org/en/v1/stdlib/Logging/).
  3. Changing the function signature by (1) introducing a new parameter (as mentioned in the previous point) and (2) changing the sync parameter form positional to keyword-only is a breaking change. Please do not change this unless you think it is useful (but even then I suggest to open a new PR).
  4. There are several smaller changes that I'm not sure if they are necessary:
    a. Changing type annotations Dict{Int,Any}()Dict{Int, Dict}()
    b. Several Int() casts
    c. Renaming variables nsamplesnsamples_block, index[id]curr_idx
    d. Style changes (2, 3, 4, 6)(2,3,4,6)
    e. Inconsistent variable naming (both dtype and T is used), and nsamples and index[id] have different names in the second pass (nsamples_block and curr_idx)
    f. The type-specific allocation logic could handle String with the same code as before
    g. No newline at end of file

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