-
Notifications
You must be signed in to change notification settings - Fork 2
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
Support figure kwargs in FlatVoxelViewer #139
Conversation
src/ess/dream/diagnostics.py
Outdated
@@ -149,8 +155,7 @@ def _flat_voxel_figure( | |||
data: sc.DataArray, | |||
horizontal_dim: str, | |||
vertical_dim: str, | |||
*, | |||
rasterized: bool = True, | |||
**kwargs: object, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why object
instead of Any
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know, it's object
elsewhere in the file so I just used that. I can change all to Any
if you like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is better. Not sure if it actually makes a difference in this case, but if we pass the kwargs
to a function that doesn't take object
arguments the type checker might be smart enough to yell at us.
@@ -182,7 +187,7 @@ def _flat_voxel_figure( | |||
h_labels = [str(value) for value in h_coord.values] | |||
v_labels = [str(value) for value in v_coord.values] | |||
|
|||
fig = flat.plot(rasterized=rasterized, cbar=True) | |||
fig = flat.plot(**kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we loose the cbar=True
argument now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but I don't think it's needed? It works without it 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're confident it makes no difference then that's good enough for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also fine with the change
@@ -182,7 +187,7 @@ def _flat_voxel_figure( | |||
h_labels = [str(value) for value in h_coord.values] | |||
v_labels = [str(value) for value in v_coord.values] | |||
|
|||
fig = flat.plot(rasterized=rasterized, cbar=True) | |||
fig = flat.plot(**kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also fine with the change
This enables e.g.
FlatVoxelViewer(dg, figsize=(10, 8), cmap='magma')
Fixes #138