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

[MRG] ENH: Added kwargs options to plot_spikes_hist #732

Merged
merged 3 commits into from
Apr 12, 2024

Conversation

samadpls
Copy link
Contributor

Closes #186

  • Enhance plot_spikes_hist() function with kwargs options

@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.67%. Comparing base (d28ddb7) to head (e28e70d).

❗ Current head e28e70d differs from pull request most recent head 2d581ff. Consider uploading reports for the commit 2d581ff to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #732   +/-   ##
=======================================
  Coverage   92.67%   92.67%           
=======================================
  Files          27       27           
  Lines        4928     4928           
=======================================
  Hits         4567     4567           
  Misses        361      361           

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

@rythorpe
Copy link
Contributor

Looks good @samadpls. You'll need to update the API section of whats_new.rst as well. Any other concerns/suggestions here @jasmainak @ntolley?

@samadpls samadpls changed the title [ENH] Added kwargs options to plot_spikes_hist ENH: Added kwargs options to plot_spikes_hist Mar 25, 2024
hnn_core/viz.py Outdated Show resolved Hide resolved
@jasmainak
Copy link
Collaborator

You'll also need to update the tests @samadpls

but your priority at this point should be finalizing the GSoC proposal. Are you on the same page with @ntolley and @rythorpe ?

@samadpls
Copy link
Contributor Author

You'll also need to update the tests @samadpls

but your priority at this point should be finalizing the GSoC proposal. Are you on the same page with @ntolley and @rythorpe ?

Sure, I'll update the test too.
Yes, @ntolley has given approval to upload it, and I'll do that today.

@samadpls samadpls requested a review from jasmainak March 26, 2024 20:24
doc/whats_new.rst Outdated Show resolved Hide resolved
@samadpls samadpls requested a review from jasmainak March 27, 2024 07:10
@rythorpe
Copy link
Contributor

It looks like you need to update your master branch and then do an interactive rebase to resolve conflicts. Have you done something like this before @samadpls?

@samadpls
Copy link
Contributor Author

It looks like you need to update your master branch and then do an interactive rebase to resolve conflicts. Have you done something like this before @samadpls?

Sure, Yes, I've done that before.

@rythorpe
Copy link
Contributor

Cool, just don't forget to backup your local branch in case things go awry. And please reach out if you run into any issues!

@rythorpe
Copy link
Contributor

rythorpe commented Apr 2, 2024

FYI @samadpls, it's best practice in this project to rebase your branch onto your updated fork of master rather than merging. Rebasing, though tricky at times, maintains a linear history of everything that was introduced in each commit and allows us to do a rebase + fast-forward merge onto master when the PR is complete.

@jasmainak
Copy link
Collaborator

I recommend updating whats_new.rst just before merge because that's a file everyone touches in their pull request. It will reduce likelihood of conflicts. Squashing commits or cherry-picking can also help with a tricky rebase

@samadpls samadpls requested a review from jasmainak April 2, 2024 10:44
@jasmainak
Copy link
Collaborator

Can you squash your commits and rebase?

@samadpls
Copy link
Contributor Author

samadpls commented Apr 2, 2024

Could you please guide me on performing a rebase and squash task?
Currently, I have been following these steps:
First, I run the command git rebase -i HEAD~7.
Then, I modify the pick commands to squash in the interactive rebase editor, save the changes, and proceed. However, when I attempt to execute git push origin enh/spike-hist --force, nothing seems to happen. I may be mistaken or doing something wrong. Could you assist me with this?

@jasmainak
Copy link
Collaborator

That seems like the right process ... you must be missing something, like you are perhaps on the wrong branch or pushing to the wrong remote. Also you have 25 commits to squash here not 7 ... I could also squash and merge for you if that makes your life easier

@samadpls
Copy link
Contributor Author

samadpls commented Apr 3, 2024

I could also squash and merge for you if that makes your life easier

Thank you for your offer. Please proceed with squash and merge for me.

@rythorpe
Copy link
Contributor

rythorpe commented Apr 3, 2024

@samadpls I think the merge commits are messing you up since git-rebase handles them differently than other commits. You also have multiple nested squash commits which makes things even trickier.

Since you only touch 4 files, I'd recommend backing up your changes, removing all commits from the root of this branch (using reset or interactive rebase), and replicating your work in 1 or a handful of commits. I know this is probably annoying, but it'll be faster than untangling your merge history.

@samadpls samadpls force-pushed the enh/spike-hist branch 2 times, most recently from 70da340 to 90f5cff Compare April 4, 2024 14:35
@samadpls
Copy link
Contributor Author

samadpls commented Apr 4, 2024

Thanks a lot for reaching out and suggesting changes, @rythorpe! It worked perfectly. I finally learned how to rebase this way. 😅

@rythorpe
Copy link
Contributor

rythorpe commented Apr 5, 2024

Looks good @samadpls! In the future you won't necessarily need to squash your commits, but it made sense here to simplify things. Glad you got the rebase figured out.

As soon as you're other PR is merged (I've set it to automerge once the tests pass), you'll need to rebase onto an updated master again since there will be a conflict in whats_new.rst caused by your most recent entry.

doc/whats_new.rst Outdated Show resolved Hide resolved
parent 54bd278
author samadpls <[email protected]> 1710875966 +0500
committer samadpls <[email protected]> 1712241138 +0500

Added `kwargs` options to `plot_spikes_hist`

Signed-off-by: samadpls <[email protected]>

refactored

removed extra line
Signed-off-by: samadpls <[email protected]>
hnn_core/viz.py Outdated Show resolved Hide resolved
@jasmainak
Copy link
Collaborator

I often use git commit --amend and force push to keep the commit history clean

@samadpls samadpls requested a review from jasmainak April 8, 2024 11:49
@samadpls samadpls changed the title ENH: Added kwargs options to plot_spikes_hist [MRG] ENH: Added kwargs options to plot_spikes_hist Apr 10, 2024
@jasmainak jasmainak merged commit 9eb491d into jonescompneurolab:master Apr 12, 2024
9 checks passed
@jasmainak
Copy link
Collaborator

Thanks @samadpls !

kmilo9999 pushed a commit to brown-ccv/hnn-core-ccv that referenced this pull request Apr 15, 2024
…rolab#732)

* Updated kwargs options for plot_spikes_hist

parent 54bd278
author samadpls <[email protected]> 1710875966 +0500
committer samadpls <[email protected]> 1712241138 +0500

Added `kwargs` options to `plot_spikes_hist`

Signed-off-by: samadpls <[email protected]>

refactored

removed extra line

* fix: name typo

Signed-off-by: samadpls <[email protected]>

* fixed docstring

---------

Signed-off-by: samadpls <[email protected]>

Apply suggestions from code review

Co-authored-by: George Dang <[email protected]>
kmilo9999 pushed a commit to brown-ccv/hnn-core-ccv that referenced this pull request Apr 16, 2024
…rolab#732)

* Updated kwargs options for plot_spikes_hist

parent 54bd278
author samadpls <[email protected]> 1710875966 +0500
committer samadpls <[email protected]> 1712241138 +0500

Added `kwargs` options to `plot_spikes_hist`

Signed-off-by: samadpls <[email protected]>

refactored

removed extra line

* fix: name typo

Signed-off-by: samadpls <[email protected]>

* fixed docstring

---------

Signed-off-by: samadpls <[email protected]>

Apply suggestions from code review

Co-authored-by: George Dang <[email protected]>
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.

Add more options to plot_spikes_hist for HNN-style histograms
4 participants