Plots/Add style parameter to plot module #257
Conversation
hoyer-a
requested review from
a team,
ahms5,
f-brinkmann,
mberz,
sikersten and
tluebeck
f-brinkmann
left a comment
f-brinkmann
left a comment
There was a problem hiding this comment.
Thanks. Looks good to me. This also improved the look of the example plots. For balloon and balloon_wireframe the axes are still clipping the colorbar, but I would leave this as is for now.
github-project-automation
bot
moved this from Require review
to Reviewer Approved
in Weekly Planning
ahms5
left a comment
ahms5
left a comment
There was a problem hiding this comment.
Thanks, looks great, I wonder why if the defult plots have noch changed due to the new default for style.
|
I only generated baseline plots for the new style parameter. I would update #242 once this is merged |
|
@mberz do you want to review as well? We could merge otherwise. |
mberz
left a comment
mberz
left a comment
There was a problem hiding this comment.
Largely good to go.
Just one point: I'd suggest to import the context manager with a more descriptive name.
|
|
||
|
|
||
| def scatter(coordinates, ax=None, **kwargs): | ||
| def scatter(coordinates, ax=None, style='light', **kwargs): |
There was a problem hiding this comment.
I've opened an issue on the naming convention, which is a bit too general in my perspective, but should not be discussed in this PR.
See pyfar/pyfar#879
hoyer-a
force-pushed
the
refactor/collect_cb_changes
branch
from
8311b1e to
cd4c63a
Compare
hoyer-a
force-pushed
the
plots/add-style-parameter
branch
from
c1680dc to
7b33f27
Compare
github-project-automation
bot
moved this from Reviewer Approved
to Done
in Weekly Planning
Merges into #222
Which issue(s) are closed by this pull request?
Closes #183
Changes proposed in this pull request: