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

Bugfix issue 3230 #3316

Merged
merged 4 commits into from Apr 16, 2023
Merged

Bugfix issue 3230 #3316

merged 4 commits into from Apr 16, 2023

Conversation

jonascristens
Copy link
Contributor

Dear @mwaskom,

I would love to contribute to the amazing project, so I have tried to resolve issue 3230.

Issue: The kws dict contained a list of tuples of dashes instead of single tuple for plotting.
Solution: Revert code partially back to changes made here
Prevention: add an additional test to check for the existing issue

@mwaskom
Copy link
Owner

mwaskom commented Apr 8, 2023

Thanks for taking a crack at this bug. It looks like you've run black over the files you've modified, which makes it very difficult to assess the functional changes to the code. Can you please revert the stylistic changes so the diff just reflects the bugfix? Thanks!

@jonascristens jonascristens reopened this Apr 10, 2023
@jonascristens
Copy link
Contributor Author

The black changes have been reversed, thanks for the quick feedback!

tests/test_relational.py Outdated Show resolved Hide resolved
@mwaskom
Copy link
Owner

mwaskom commented Apr 15, 2023

Thanks but I don't think this is the right solution. Better to handle the bug where it was introduced, in the process of reconciling parameters in the body of lineplot. I am pretty sure that you just need to change this line:

https://github.com/mwaskom/seaborn/blob/master/seaborn/relational.py#L599

There are two ways to have a defined style mapping: explicitly, where the local style variable is not None, and implicitly, when using wide-form data. The change added here to allow dashes to be a literal specification of dashes when not using a style mapping forgot about the second (implicit) way. So basically I think this just needs to change the check for style is None to "style" not in p.variables.

@jonascristens
Copy link
Contributor Author

Thanks for the feedback, I've adjusted the code as requested. I now have a better understanding of what is happening exactly.

@codecov
Copy link

codecov bot commented Apr 15, 2023

Codecov Report

Merging #3316 (65f708c) into master (4f72554) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3316   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files          77       77           
  Lines       24942    24955   +13     
=======================================
+ Hits        24538    24551   +13     
  Misses        404      404           
Impacted Files Coverage Δ
seaborn/relational.py 99.68% <100.00%> (ø)
tests/test_relational.py 99.31% <100.00%> (+<0.01%) ⬆️

... and 2 files with indirect coverage changes

@mwaskom
Copy link
Owner

mwaskom commented Apr 16, 2023

Thanks!

@mwaskom mwaskom merged commit bf0cfec into mwaskom:master Apr 16, 2023
10 checks passed
@jonascristens jonascristens deleted the bugfix_3230 branch April 17, 2023 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants