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.

Sign up for GitHub

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

Jump to bottom

Adding setter for colorbar's label_str #1292

Closed
wants to merge 12 commits into from
Closed

Adding setter for colorbar's label_str #1292

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b273d58
Adding setter for label_str
macrocosme Nov 3, 2016
a4dbf05
Adding ._colourbar. to access label_str
macrocosme Nov 8, 2017
bb774cf
Adding ._colorbar.
macrocosme Nov 8, 2017
d2d03e9
Reverting change ._colorbar
macrocosme Nov 8, 2017
9e56b9a
Changing both setter and getter
macrocosme Nov 19, 2017
47060e4
Adding short docstrings
macrocosme May 4, 2018
f15ca6f
Adding ticks
macrocosme May 4, 2018
6030a92
Merge branch 'master' into master
macrocosme May 4, 2018
939917d
Add @property back
macrocosme May 7, 2018
59a9d18
Remove property/setter label_str, and rename label_str to label_text
macrocosme Jul 5, 2018
0bf9f72
Change label_str to label_text
macrocosme Jul 5, 2018
53b5114
Remove indentation
macrocosme Oct 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions vispy/scene/widgets/colorbar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,22 @@ def label(self, label):
self._colorbar.label = label

@property
def label_str(self):
"""Get the colorbar string
"""
return self._colorbar._label_str

@label_str.setter
def label_str(self, label_str):
Copy link
Contributor

Choose a reason for hiding this comment

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

@macrocosme You also need to do this change in the setter.

"""Set the colorbar string

Parameters
----------
label_str : string label to be displayed with the colorbar
"""
self._colorbar._label_str = label_str
self._update()

def ticks(self):
Copy link
Member

Choose a reason for hiding this comment

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

You lost the @property on this ticks property which is causing the errors on travis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh my... ! It's back in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for walking me through the process.

return self._colorbar.ticks

Expand Down
23 changes: 23 additions & 0 deletions vispy/visuals/colorbar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,29 @@ def label(self, label):
self._label = label
self._update()
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why it took me this wrong to realize this, but why can't all of this be done with an if statement for the regular label property? I know I've expressed my dislike of label_str as a property and I didn't see a way around it before, but why not do:

@label.setter
def label(self, label):
    if isinstance(label, str):
        self._label.text = label
    else:
        self._label = label
        self._update()

I'm not sure if the _update needs to be done in both cases since the TextVisual would call its own update. With this change the label_str property could be removed. Additionally, have you actually tested this? When does the ColorbarVisual._label_str get used? It doesn't seem to show up in the _update function.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@djhoese 👍

Copy link
Contributor Author

@macrocosme macrocosme May 7, 2018 •
edited

Choose a reason for hiding this comment

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

I was using this code here https://github.com/macrocosme/shwirl/blob/master/shwirl/extern/vispy/scene/widgets/colorbar.py without a fuss. But I should indeed sit down and properly test this. I'm travelling so it may be a few days.

Edit: I'm finally working on the clean-up. I realised I have had modified _update to include this property: https://github.com/macrocosme/shwirl/blob/15cf73631007cba45a37db698f5e06c48714e598/shwirl/extern/vispy/visuals/colorbar.py#L347

Nevertheless, I think the using self.label.text is the way to go. A bit more to come later today hopefully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As to merging the two with an if, I like it! 👍


@property
def label_str(self):
""" Get the string describing the label for the colorbar

Returns
-------
_label_str : str
Label string value
"""
return self._label_str
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, add short docstring with description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Docstrings finally added. Sorry for the wait!


@label_str.setter
def label_str(self, label_str):
""" Set the string that describes the label of the colorbar

Parameters
----------
label_str : str
string describing the colorbar
"""
self._label_str = label_str
self._update()

@property
def ticks(self):
""" The vispy.visuals.TextVisual associated with the ticks
Expand Down