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

Improve temporally_align_data_interfaces #1162

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

Conversation

pauladkisson
Copy link
Member

@pauladkisson pauladkisson commented Dec 17, 2024

Fixes #1161

Unfortunately, this will break back-compatibility for anyone who implemented their own version of temporally_align_data_interfaces w/o re-implementing run_conversion. So, probably needs to be slated for 0.7.0.

@h-mayorquin, do you have a list of TODOs for 0.7.0 or a branch or something so that it doesn't get lost?

@h-mayorquin
Copy link
Collaborator

@h-mayorquin, do you have a list of TODOs for 0.7.0 or a branch or something so that it doesn't get lost?

No, but after the next release, we will make breaking changes so the next should be 0.7.0 anyway.

@h-mayorquin
Copy link
Collaborator

h-mayorquin commented Dec 18, 2024

I slept on this, and here is my take:

  • I can't find any good argument against having this, and it seems that this should have been the case from the beginning. Plus, it should make your work easier, right?
  • I am not a heavy user of this function; I usually work directly at the interface level. I think you should bring it up in one of the team meetings to get feedback, especially from Ben, who, if I remember correctly, is a user of this and likely wants more input on high-level API decisions.
  • It is worth considering whether some judicious use of kwargs and/or positional-only arguments could avoid the backward compatibility break not necessarily here, but at least in other situations where we might need to change the function signature in the future.
  • I still think the accompanying issue requires you to write a concrete case so better reasons for the change are documented somewhere.

So, I am a +1 on this. It is unclear to me what kind of test, if any, this might require.

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.75%. Comparing base (96dfdff) to head (77659de).
Report is 12 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1162      +/-   ##
==========================================
+ Coverage   90.69%   90.75%   +0.06%     
==========================================
  Files         129      129              
  Lines        8189     8286      +97     
==========================================
+ Hits         7427     7520      +93     
- Misses        762      766       +4     
Flag Coverage Δ
unittests 90.75% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/neuroconv/nwbconverter.py 94.96% <100.00%> (ø)

... and 3 files with indirect coverage changes

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