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

Add posibility to turn off color bar in 2d plot #1343

Merged
merged 1 commit into from
May 15, 2018

Conversation

aringh
Copy link
Member

@aringh aringh commented May 9, 2018

Example on how #1342 could be implemented. I found no tests for odl.util.graphics, but a minimal working example that shows how it works is given by

import odl
space = odl.uniform_discr([0,0], [1,1], [256,256])
x = odl.util.noise_element(space)
x.show()
x.show(show_2d_color_bar=False)

which gives the following images
figure_1
figure_3

Is this desirable? Or is it only adding unnecessary complexity to plotting?

@aringh
Copy link
Member Author

aringh commented May 9, 2018

They fail is due to doc build fail. This is observed also in (at least) #1321 and #1340

@adler-j
Copy link
Member

adler-j commented May 9, 2018

I like it overall, but perhaps we should change the name of there parameter?

Something like show_colorbar or simply colorbar?

@adler-j
Copy link
Member

adler-j commented May 9, 2018

And indeed something is up with the build server. Perhaps a Sphinx issue. Do you have any idea @kohr-h?

@kohr-h
Copy link
Member

kohr-h commented May 9, 2018

There's been an update do travis-sphinx on PyPI which seems to have broken things. Need to test it locally with more output.

@kohr-h
Copy link
Member

kohr-h commented May 9, 2018

Remove the "<1.7" part from the Sphinx install command, then everything should be fine. The new version of travis-sphinx doesn't work with Sphinx 1.6.x, possibly due to a patch by me 🙄

@kohr-h
Copy link
Member

kohr-h commented May 9, 2018

Remove the "<1.7" part from the Sphinx install command, then everything should be fine. The new version of travis-sphinx doesn't work with Sphinx 1.6.x, possibly due to a patch by me roll_eyes

After rebasing, the error should be fixed.

@kohr-h
Copy link
Member

kohr-h commented May 9, 2018

Something like show_colorbar or simply colorbar?

Definitely colorbar, since that allows us to later enable other types than bool, for example an instance of Colorbar or a string, if we ever decide to implement such a thing.

That said, feature creep is a real issue here. I'd say yes to changes that are very simple to implement with an additional flag in an if ... statement, but be cautious with changes that require more code. After all, this method is just a quick 'n dirty plotter, although I realize that lots of projects have these tools: started as a quick hack and became the most heavily used tool.

@aringh aringh force-pushed the issue-1342__color_bar_2d_plot branch 2 times, most recently from f922bb9 to 1ea77c2 Compare May 11, 2018 14:22
@aringh
Copy link
Member Author

aringh commented May 11, 2018

Changed the name and rebased. I will leave the PR hanging a little longer if @adler-j or @kohr-h have any final comments. Otherwise I will merge it sometime early next week.

@@ -160,6 +160,11 @@ def show_discrete_data(values, grid, title=None, method='',
axis_fontsize : int, optional
Fontsize for the axes. Default: 16

colorbar : bool, optional
Argument relevant for 2d plots using ``method`` 'imshow'. If the plot
Copy link
Member

Choose a reason for hiding this comment

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

2 quick comments:

  • ``method`` 'imshow' -> ``method='imshow'``
  • "If" can't start a main clause, I guess "Whether" would be more correct. But generally, explanations like "If ``True``, include a colorbar in the plot." are preferred.

@aringh aringh force-pushed the issue-1342__color_bar_2d_plot branch from 1ea77c2 to 94cb2cc Compare May 15, 2018 07:23
@aringh
Copy link
Member Author

aringh commented May 15, 2018

Great! I will merge after the tests

@aringh aringh merged commit 826904f into odlgroup:master May 15, 2018
@aringh aringh deleted the issue-1342__color_bar_2d_plot branch June 14, 2018 20:22
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.

3 participants