-
Notifications
You must be signed in to change notification settings - Fork 22
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
Drop casting to int in units table and introduce fine control over null values in units_table
#989
Conversation
… fix_bug_with_mapping
… fix_bug_with_mapping
… fix_bug_with_mapping
Co-authored-by: Ben Dichter <[email protected]>
…_in_units_table' into drop_casting_to_int_in_units_table
nwbfile = io.read() | ||
self._test_analyzer_write(self.single_segment_analyzer, nwbfile) | ||
self.assertIn("ElectricalSeriesRaw", nwbfile.acquisition) | ||
# def test_write_sorting_analyzer_to_file(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the plan here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move those to add_methods
. There is an error there that I can't really pin down but the test is too wide for me to figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to clarify, your source code changes here break a current test for write_sorting_analyzer
, but we don't know why so the plan is to comment it out and debug separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. I will solve that after The function write_sorting_analyzer is doing too many things. I want to break appart those tests to the format of the other adds where we test the construction of the nwbfile separated from the wriging and not all bundled up like we do here.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #989 +/- ##
==========================================
- Coverage 91.43% 91.25% -0.19%
==========================================
Files 127 127
Lines 7540 7555 +15
==========================================
Hits 6894 6894
- Misses 646 661 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Should close ##960 should come after #985