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

Do not mutate scalar_ bar_args #1357

Merged
merged 6 commits into from
Jun 6, 2021

Conversation

MatthewFlamm
Copy link
Contributor

Overview

This PR copies scalar_bar_args to avoid mutating the passed in variable in add_mesh and add_volume.

Fixes #1353

Details

Tests added for future regression testing.

@codecov
Copy link

codecov bot commented Jun 5, 2021

Codecov Report

Merging #1357 (4f6e9af) into master (9c15306) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1357   +/-   ##
=======================================
  Coverage   90.06%   90.07%           
=======================================
  Files          58       58           
  Lines       12265    12267    +2     
=======================================
+ Hits        11047    11049    +2     
  Misses       1218     1218           

pyvista/plotting/plotting.py Outdated Show resolved Hide resolved
Copy link
Member

@adeak adeak left a comment

Choose a reason for hiding this comment

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

Mostly musing or bikeshedding remarks. The only "hard" suggestion is the show() removal in the tests. Apart from that I think this is great, so approving.

if "scalar" in kwargs:
raise TypeError("`scalar` is an invalid keyword argument for `add_mesh`. Perhaps you mean `scalars` with an s?")
assert_empty_kwargs(**kwargs)

# Avoid mutating input
if scalar_bar_args is None:
scalar_bar_args = {}
Copy link
Member

@adeak adeak Jun 5, 2021

Choose a reason for hiding this comment

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

I know this is unrelated to the PR, but I'm wondering if it makes sense that n_colors is not set for add_volume. It might not hurt to check that n_colors affects the colorbar for add_mesh .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would also be nice to extract out the common functionality for these things into a helper to make it consistent. I think this should be a different PR as it is about functionality (with a possibly breaking change) rather than fixing an unintended effect.

pyvista/plotting/plotting.py Show resolved Hide resolved
tests/plotting/test_plotting.py Outdated Show resolved Hide resolved
tests/plotting/test_plotting.py Outdated Show resolved Hide resolved
@akaszynski
Copy link
Member

I'm merging this for the 0.31.0 release. For the record, two potential unresolved issues for future PRs.

@akaszynski akaszynski merged commit 6a48685 into pyvista:master Jun 6, 2021
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.

title key added to scalar_bar_args in add_mesh
3 participants