jonescompneurolab / hnn-core

Simulation and optimization of neural circuits for MEG/EEG source estimates
https://jonescompneurolab.github.io/hnn-core/
BSD 3-Clause "New" or "Revised" License
54 stars 52 forks source link

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

Closed samadpls closed 5 months ago

samadpls commented 6 months ago

Closes #186

codecov-commenter commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

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

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

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rythorpe commented 6 months ago

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

jasmainak commented 6 months ago

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 commented 6 months ago

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.

rythorpe commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

jasmainak commented 6 months ago

Can you squash your commits and rebase?

samadpls commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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

rythorpe commented 6 months ago

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.

jasmainak commented 6 months ago

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

jasmainak commented 5 months ago

Thanks @samadpls !