-
Notifications
You must be signed in to change notification settings - Fork 18
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
Chang to merge #76
Chang to merge #76
Conversation
-bug fixes -numerous changes for PEP compliance
…h bipolar electrodes
…tes that break the code at the current moment
…ds improvement though
…licking "Run Test"
…" if the key doesn't exist
…res with confidence intervals)
…ing the event detection gui
Event detection
…numpy object, this needed to be changed with update of pynwb to 1.1.2
fix definitions in electrode table to be strings and not any sort of …
# Conflicts: # README.md # ecogvis/functions/misc_dialogs.py # ecogvis/functions/nwb_copy_file.py # ecogvis/functions/subFunctions.py # ecogvis/signal_processing/detect_events.py # ecogvis/signal_processing/processing_data.py # ecogvis/signal_processing/resample_clone.py # setup.py
It looks like there were some issues merging these together. I'll look into these. |
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.
I see weird line breaks in many places, feel like an editor bug from someone
I wouldn't merge it now. I ran it and has some weird bugs in some elements. Update |
Maybe everyone should check if this version is working as expected for their current workflows? Before merging to the master A reference for testing the PR locally: most important part, on your local repo directory: |
This seems trivial but it's my workflow to run ecogVIS from inside a python console in PyCharm as: This now breaks, because of the Objections? EDIT: I now realize this is a problem with |
It looks like some issues came up in the merge beyond weird formatting. I think this is going to few days to sort out. We'll be in touch when it's ready to test. |
I fixed some test errors and most are passing now.
|
@luiztauffer Thanks. Yes, I suspect there have been some changes to the event detection function that will break numerical equivalence. As long as the new results are reasonable we can just replace the array they are being compared to. I'd like to strictly adhere to the tests passing before merge so that we can continue to rely in CI to catch our mistakes. I'll take a look and see if I can repair the tests. |
- add example mic and speaker data for test
Codecov Report
@@ Coverage Diff @@
## master #76 +/- ##
=======================================
Coverage 19.59% 19.59%
=======================================
Files 23 23
Lines 4949 4949
=======================================
Hits 970 970
Misses 3979 3979
Continue to review full report at Codecov.
|
@bendichter |
@luiztauffer I went through and handled all of the merge conflicts with the Chang Lab's master branch, but I wanted to run these changes by you before merging. When you have time, can you look at these and make sure that it doesn't break anything on your end?