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

[ENH] Added n_con arg to function call #133

Merged
merged 5 commits into from
Mar 13, 2023

Conversation

Avoide
Copy link
Contributor

@Avoide Avoide commented Mar 9, 2023

PR Description

Moved the hard-coded variable n_con to be one of the arguments for the function call. To enable choosing the number of connections shown, instead of always 20. Added a default option, to make it compatible with previous usage.

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>"

Copy link
Member

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

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

Can you fix the style error in make pep?

You'll need to install flake8, pydocstyle and codespell

@Avoide
Copy link
Contributor Author

Avoide commented Mar 10, 2023

I fixed the style errors associated with the file I changed, but the test is not passing due to errors in two other files:
envelope.py and smooth.py.
I already corrected those codespell errors in my other PR, but does it make sense to also correct in this PR?

@adam2392
Copy link
Member

I already corrected those codespell errors in my other PR, but does it make sense to also correct in this PR?

Now that I've merged in the other PR, you can "rebase your changes on top of main", or "merge in main". Lmk if you don't know how to do that.

Can you also add a changelog entry to doc/whats_new.rst? You can see the previous releases file for a format of how to do so.

@adam2392
Copy link
Member

Oh actually, I forgot I can just merge in main from the PR itself here with a button click, so now you can just run git pull to get the latest changes on your branch

@Avoide
Copy link
Contributor Author

Avoide commented Mar 13, 2023

Thanks, I updated whats_net and added an entry into authors. Hopefully the link should work.

@adam2392 adam2392 merged commit b36ce8a into mne-tools:main Mar 13, 2023
@adam2392
Copy link
Member

Thanks @Avoide !

tsbinns pushed a commit to tsbinns/mne-connectivity that referenced this pull request Dec 15, 2023
* Added n_con arg to function call

---------
Co-authored-by: Adam Li <[email protected]>
Authored-by: Qinliang Li <[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.

2 participants