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.

Sign up for GitHub

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

Jump to bottom

fix: change ci param to errorbar in sns-catplot #524

Merged
merged 7 commits into from
Oct 12, 2023
Merged

fix: change ci param to errorbar in sns-catplot #524

merged 7 commits into from
Oct 12, 2023

Conversation

Busato
Copy link
Contributor

@Busato Busato commented Oct 12, 2023 •
edited
Loading

What does this implement/fix?

Two tests were failing because of a deprecation of the ci parameter in sns.catplot function.

image

They were changed to errorbar instead that do the same thing when using None (both of the were using it)

Contributed with ❤️ by AE Studio

@codecov
Copy link

codecov bot commented Oct 12, 2023 •
edited
Loading

Codecov Report

Merging #524 (91b407f) into main (047a27a) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is n/a.

❗ Current head 91b407f differs from pull request most recent head acbc32f. Consider uploading reports for the commit acbc32f to get more accurate results

@@           Coverage Diff           @@
##             main     #524   +/-   ##
=======================================
  Coverage   95.22%   95.22%           
=======================================
  Files          69       69           
  Lines        2868     2868           
  Branches      412      412           
=======================================
  Hits         2731     2731           
  Misses         66       66           
  Partials       71       71

@larsoner
Copy link
Member

CI failures appear unrelated, let me fix those in a separate PR then merge main into this branch, then should be good to go. Thanks in advance @Busato !

larsoner
larsoner reviewed Oct 12, 2023
View reviewed changes
doc/changelog.rst
@@ -197,3 +197,4 @@ Enhancements
.. _Samuel Powell: https://github.com/samuelpowell
.. _Johann Benerradi: https://github.com/HanBnrd
.. _Florin Pop: https://github.com/florin-pop
.. _Nicolas Busato: https://github.com/Busato
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a line under the v0.6.dev0 heading toward the top?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@larsoner do you mean a line saying what the PR changed?

Copy link
Member

Choose a reason for hiding this comment

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

Yep!

@larsoner
Copy link
Member

You can now also remove this line that I added https://github.com/mne-tools/mne-nirs/pull/525/files#diff-15c86dcddb07192eeb2d84db2a5dfca48e3837471adbbd9b3e05425b44fce485R49 if you want to make sure it has been fixed everywhere

@Busato Busato requested a review from larsoner October 12, 2023 17:55
@rob-luke
Copy link
Member

Thanks @Busato 🎉

@rob-luke rob-luke merged commit a900d8e into mne-tools:main Oct 12, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larsoner larsoner Awaiting requested review from larsoner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Busato @larsoner @rob-luke