Aggregate events.tsv for invalid entities (and rec)

[octomike]

We currently get two (empty) events.tsv files for our rec-norm and rec-raw bold pairs. This is redundant and should be aggregated into an events file without a rec entity altogether (the spec allows it, but I would argue that events are independent of reconstruction methods, right?).

I had a look at the code and cleaned it up a little to utilize BIDSFile.parse() instead of manual splits around _echo. I also realized that there are other entities valid for the bold, but invalid for the corresponding events.tsv and remove them. For that I added a drop() and a trivial __contains__() method to BIDSFile to easily remove an entity from the filename and have concise tests.

This should also fix #849

---

I believe the __contains__ is an intuitive add-on, but it might lead to confusion with code like this:

bidsfile = BIDSFile.parse(bold)
if 'rec' in bidsfile:
  ...

Under the hood it will now check if rec is in the _entities dictionary, but it might read as: Is rec literally in the string representation of the filename? I'm not sure if you want to avoid this little trap.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.73%. Comparing base (e088d2c) to head (121ad10).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #864      +/-   ##
==========================================
+ Coverage   83.64%   83.73%   +0.09%     
==========================================
  Files          42       42              
  Lines        4506     4525      +19     
==========================================
+ Hits         3769     3789      +20     
+ Misses        737      736       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[yarikoptic]

THANK YOU for the PR -- we should fix this indeed. BUT I think we should not sanitize rec, and also tune code a bit

[yarikoptic-gitmate]

One heads-up when applying the suggestions above: turning drop() into an exception is a two-file change — the existing test_BIDSFile currently asserts a warning is logged on a missing-entity drop, so it will start failing once drop() raises.

What needs updating in test_bids.py

The current block relies on the warning behavior:

caplog.set_level(logging.WARNING)
for entity in ['dir', 'not_existing']:
    caplog.clear()
    my_bids_file.drop(entity)
    assert len(caplog.records) == 1
    assert f"does not contain entity '{entity}'" in caplog.records[0].message

With drop(entity, missing_ok=False) raising ValueError, this should become something like:

for entity in ["dir", "not_existing"]:
    with pytest.raises(ValueError, match=f"does not contain entity {entity!r}"):
        my_bids_file.drop(entity)
    # and the missing_ok escape hatch is a no-op:
    my_bids_file.drop(entity, missing_ok=True)

The import logging / caplog fixture then become unused and can be dropped.

Also worth adding an echo (and now chunk) case to test_populate_aggregated_jsons_events, since the multi-echo path is exactly the code being rewritten here.

---

Generated by Claude Code

[octomike]

Hey @yarikoptic - thank you so much for that in-depth review. I feel this was much more work than what I did initially with the PR 😅

Your and Claude's points are very valid and I applied most of them verbatim. Fixed the tests as well.

It's just unfortunate that the reason I dug into this is now off the table with rec- being untouched. But, well, worth it anyway!

[yarikoptic]

thanks @octomike ! So, you are still having multiple _rec- files for the same sequence and it is not just a matter of removing _rec-*_events.tsv ones while leaving also existing ones without _rec-? I wonder if in follow up PR we could add some bids-specific option like norec-events which would alter behavior here to the one you desire ... ;-)

github actions is ATM a bust , so I will run tests locally but also wait for a day or so

[yarikoptic]

re RTD -- if you update branch on top of master (merge or rebase), should render ok

[octomike]

thanks @octomike ! So, you are still having multiple _rec- files for the same sequence and it is not just a matter of removing _rec-*_events.tsv ones while leaving also existing ones without _rec-? I wonder if in follow up PR we could add some bids-specific option like norec-events which would alter behavior here to the one you desire ... ;-)

Yeah, I'll think about it. I'm inclined to just have this artifact fixed after conversion now that the possible neat solution is off the table. Will check with the "customer" (researcher).

github actions is ATM a bust , so I will run tests locally but also wait for a day or so

Don't sweat it.

[yarikoptic]

Suggested change

	def drop(self, entity: str, missing_ok: bool=False) -> None:
	def drop(self, entity: str, missing_ok: bool = False) -> None:

to please linters

[yarikoptic]

hm, I need to sleep on it but it says

may be you didn't allow contributors to push to you branch and that's the reason @octomike ?