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

[WIP] Detector Comparison Charts #54

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

Conversation

locook03
Copy link

@locook03 locook03 commented Sep 20, 2023

Thanks for contributing. If this is your first time,
make sure to read contributing.md

PR Description

  • Implements detector comparison functions.
  • Adds functions to plot the comparisons to a figure.
  • Adds visualization tool for HFO highlighting.

Merge checklist

Maintainer, please confirm the following before merging:

  • All comments resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • PR description includes phrase "closes <#issue-number>"

@adam2392
Copy link
Member

@locook03 what are you using the package for and are you trying to add new detectors/etc.?

I haven't really been maintaining, but if there's interest, I can patch up some of the dev issues.

@locook03
Copy link
Author

Hey @adam2392 , I'm working with Patrick Myers to complete the comparison functions and eventually to add a data pipeline and additional detectors. I've patched up some of the dev issues, but I would appreciate it if you reviewed as well!

@adam2392
Copy link
Member

Hey @adam2392 , I'm working with Patrick Myers to complete the comparison functions and eventually to add a data pipeline and additional detectors. I've patched up some of the dev issues, but I would appreciate it if you reviewed as well!

Okay I will try to find some time to get the CIs working and running again. I will also have to upgrade the entire package to Python 3.8+ as Python 3.6/3.7 have reached End-of-life.

@adam2392
Copy link
Member

adam2392 commented Sep 21, 2023

Kay @locook03 and @pmyers16 I've updated the repo to Python3.8+, fixed CIs and fixed some bugs in #55. You'll need to merge with main and resolve conflicts.

@adam2392
Copy link
Member

It would help if you create a GH issue, or put it in the PR description what exactly the problem it is you're trying to solve with this PR.

"""

# If no axis is provided, the current axis will be used,
if ax == None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this works if there is no figure. The way I have always seen this done is:

if ax is None:
fig, ax = plt.subplots(...)

that way you also have control over things like figure size

ax.plot()
return ax
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for consistency with the other plotting functions, I would return both fig, ax and then I would make the fig, ax as optional inputs for plot_hfo_events

ax.plot()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this line do anything?

@adam2392
Copy link
Member

Feel free to change and update as is and ping me if I need to look at anything. Otherwise, I'll give you guys free reign to try stuff out :)

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.

None yet

4 participants