Adding setter for colorbar's label_str #1292

macrocosme · 2016-11-03T22:41:01Z

Minor fix related to issue #1251 & #1271

#1271

Minor fix related to issue vispy#1251 & vispy#1271 vispy#1271

larsoner · 2017-05-04T16:21:19Z

This needs rebase

kmuehlbauer · 2017-11-07T07:57:43Z

@macrocosme Can you please rebase this. If you do not have time, I can take over. Just let me know.

kmuehlbauer

@macrocosme Let's get this in, finally. Sorry for the delay.

kmuehlbauer · 2017-11-07T07:58:51Z

vispy/scene/widgets/colorbar.py

@@ -133,6 +133,15 @@ def label(self, label):
        self._colorbar.label = label

    @property
+    def label_str(self):
+        return self._label_str


Please add a short docstring with the description.

Please change this to self._colorbar.label_str.

kmuehlbauer · 2017-11-07T07:59:20Z

vispy/visuals/colorbar.py

@@ -597,6 +597,15 @@ def label(self, label):
        self._update()

    @property
+    def label_str(self):
+        return self._label_str


Same here, add short docstring with description.

Docstrings finally added. Sorry for the wait!

djhoese · 2017-11-07T13:48:22Z

I know this is out of the scope for this PR most likely but as a style guideline we should probably avoid keyword arguments with datatypes in them label_str. IMO the visual should have a self._label_visual and store the string as self._label. Or at least name the keyword argument label.

Also I'm a little confused about the the colorbar widget with this property. I think it is supposed to be self._colorbar.label_str.

djhoese · 2017-11-07T13:49:47Z

vispy/scene/widgets/colorbar.py

@@ -133,6 +133,15 @@ def label(self, label):
        self._colorbar.label = label

    @property
+    def label_str(self):
+        return self._label_str


Please change this to self._colorbar.label_str.

djhoese · 2017-11-07T13:50:06Z

vispy/scene/widgets/colorbar.py

+
+    @label_str.setter
+    def label_str(self, label_str):
+        self._label_str = label_str


Please change this also to self._colorbar.label_str.

Will do as soon as I can.

kmuehlbauer · 2017-11-07T13:56:26Z

I know this is out of the scope for this PR most likely but as a style guideline we should probably avoid keyword arguments with datatypes in them label_str.

@davidh-ssec This change was introduced with #979, there was surely some logic behind to do this. @bollu could you chime in, when you find the time?

kmuehlbauer · 2017-11-07T13:57:56Z

IIRC this was to differentiate the self._label (TextVisual) from the label string itself.

djhoese · 2017-11-07T14:21:45Z

Yes of course, but that's why I think in the future we need to be careful about what the user sees and what they don't. Having the keyword argument label is a lot cleaner than label_str. This means self._label becomes self._label_visual. However, I've been finding more and more that user's need to access these sub-visuals by name. This either means that the interface is not complete enough or we need to make less "private" attributes: self.label versus self._label

@kmuehlbauer

Acting on @kmuehlbauer request

@kmuehlbauer

Acting on @kmuehlbauer’s request

macrocosme · 2017-11-08T10:52:01Z

I have added the requested changes.

kmuehlbauer · 2017-11-08T10:52:47Z

@macrocosme Thanks. Are ready for rebasing this to resolve the conflicts?

kmuehlbauer · 2017-11-08T11:01:16Z

vispy/visuals/colorbar.py

@@ -598,7 +598,7 @@ def label(self, label):

    @property
    def label_str(self):
-        return self._label_str


@macrocosme This will not work. There is no self._colorbar in vispy/visuals/colorbar.py

I meant to just add some docstring here.

Please revert this change.

I'm at a conference. I'll get back to it when I have a bit more time to concentrate! : )
This should be soon.

kmuehlbauer · 2017-11-08T11:04:51Z

vispy/scene/widgets/colorbar.py

@@ -134,7 +134,7 @@ def label(self, label):

    @property
    def label_str(self):
-        return self._label_str
+        return self._colorbar._label_str

    @label_str.setter
    def label_str(self, label_str):


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

@kmuehlbauer

As requested by @kmuehlbauer

Changing to set/get using _colorbar Also includes a simple docstring

macrocosme · 2017-11-19T08:49:34Z

@kmuehlbauer @davidh-ssec Okay. I have now changed both setter and getter in widgets/colorbar.py, and also added docstrings to both files (widgets/colorbar.py and visuals/colorbar.py).

djhoese · 2018-05-04T19:28:31Z

@macrocosme Awesome. Looks like you'll have to rebase or merge the current master branch to resolve some conflicts and get tests to pass.

macrocosme · 2018-05-04T23:16:12Z

@djhoese Done!

djhoese · 2018-05-06T14:32:54Z

vispy/scene/widgets/colorbar.py

+        """
+        self._colorbar._label_str = label_str
+        self._update()
+
    def ticks(self):


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

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

Thanks for walking me through the process.

djhoese

Have you actually tested this?

djhoese · 2018-05-07T02:25:42Z

vispy/visuals/colorbar.py

@@ -604,6 +604,29 @@ def label(self, label):
        self._label = label
        self._update()


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?

@djhoese 👍

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.

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

- Rename label_str to label_text - Change label setter to check for str.

macrocosme · 2018-07-05T01:41:57Z

@kmuehlbauer @djhoese I finally made the changes, and tested it. Note that I have renamed label_str to label_text as it was discussed that having str in the variable name isn't ideal.

djhoese · 2020-08-31T21:50:41Z

I'm not sure what happened with this PR and I'm extremely sorry we lost track of it (I think your last comment was right before SciPy 2018 started). I see the tests passed and things are basically as we've described. I know you will hate me but now that the setter can handle a string is there a reason we can't have label= in the __init__ method?

@kmuehlbauer Thoughts?

kmuehlbauer · 2020-09-01T05:17:47Z

@djhoese I'm not sure I understand. You mean instead of self._label=... it should be self.label=...?

macrocosme · 2020-09-01T07:51:28Z

@djhoese @kmuehlbauer It's okay. These things happen. Nearly 4 years in, I will admit I thought this project had slowly stopped. I will let these final changes to you two if that is okay.

djhoese · 2020-09-01T13:13:04Z

@kmuehlbauer The keyword argument in master is label_str. Is there a reason it couldn't (or shouldn't) be just label?

kmuehlbauer · 2020-09-01T13:18:44Z

The keyword argument in master is label_str. Is there a reason it couldn't (or shouldn't) be just label?

Yes, sure, users which use matplotlib would expect this. 👍

kmuehlbauer · 2020-09-01T13:24:57Z

@djhoese This seems impossible to merge master into this branch. Should we extract all needed commits and rebase on current master? Then this can run on the recent CI?

djhoese · 2020-09-01T13:52:25Z

It is impossible to merge master because it is @macrocosme 's master branch (instead of a separate "feature" branch). GitHub doesn't give maintainers implicit permission to fork's master branches. We'll need to take the fork's master branch and create our own branch on our own fork, merge vispy master, then make a new PR.

djhoese · 2021-06-09T12:30:24Z

This has been replaced by #2057 and don't worry I kept you as the author of the commit for your work in this PR (had to copy some files around to get the commit history to not be garbage). Thanks @macrocosme!

Adding setter for label_str

b273d58

Minor fix related to issue vispy#1251 & vispy#1271 vispy#1271

kmuehlbauer requested changes Nov 7, 2017

View reviewed changes

djhoese requested changes Nov 7, 2017

View reviewed changes

macrocosme added 2 commits November 8, 2017 12:49

Adding ._colourbar. to access label_str

a4dbf05

Acting on @kmuehlbauer request

Adding ._colorbar.

bb774cf

Acting on @kmuehlbauer’s request

kmuehlbauer reviewed Nov 8, 2017

View reviewed changes

kmuehlbauer mentioned this pull request Nov 8, 2017

Underscore for private objects #399

Closed

macrocosme added 2 commits November 8, 2017 15:58

Reverting change ._colorbar

d2d03e9

As requested by @kmuehlbauer

Changing both setter and getter

9e56b9a

Changing to set/get using _colorbar Also includes a simple docstring

sylm21 mentioned this pull request Mar 11, 2018

GLSL shader for iso-filles value in colorbar and meshVisual #1445

Open

Adding short docstrings

47060e4

macrocosme added 2 commits May 4, 2018 19:14

Adding ticks

f15ca6f

Merge branch 'master' into master

6030a92

djhoese requested changes May 6, 2018

View reviewed changes

Add @Property back

939917d

djhoese reviewed May 7, 2018

View reviewed changes

macrocosme added 2 commits July 5, 2018 10:50

Remove property/setter label_str, and rename label_str to label_text

59a9d18

Change label_str to label_text

0bf9f72

- Rename label_str to label_text - Change label setter to check for str.

macrocosme changed the title ~~Adding setter for label_str~~ Adding setter for colorbar's label_str Jul 5, 2018

Remove indentation

53b5114

djhoese closed this Apr 28, 2021

djhoese deleted the branch vispy:main April 28, 2021 19:00

djhoese reopened this Apr 28, 2021

djhoese closed this Apr 28, 2021

djhoese deleted the branch vispy:main April 28, 2021 19:27

djhoese reopened this Apr 28, 2021

djhoese changed the base branch from master to main April 28, 2021 20:57

This was referenced Jun 9, 2021

Add setter for colorbar label text #2056

Closed

Add setter for colorbar label text #2057

Merged

djhoese closed this Jun 9, 2021

		@@ -604,6 +604,29 @@ def label(self, label):
		self._label = label
		self._update()

Adding setter for colorbar's label_str #1292

Adding setter for colorbar's label_str #1292

Conversation

macrocosme commented Nov 3, 2016

larsoner commented May 4, 2017

kmuehlbauer commented Nov 7, 2017

kmuehlbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djhoese commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmuehlbauer commented Nov 7, 2017

kmuehlbauer commented Nov 7, 2017

djhoese commented Nov 7, 2017

macrocosme commented Nov 8, 2017

kmuehlbauer commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macrocosme commented Nov 19, 2017 • edited

djhoese commented May 4, 2018

macrocosme commented May 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djhoese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macrocosme May 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macrocosme commented Jul 5, 2018 • edited

djhoese commented Aug 31, 2020

kmuehlbauer commented Sep 1, 2020

macrocosme commented Sep 1, 2020

djhoese commented Sep 1, 2020

kmuehlbauer commented Sep 1, 2020

kmuehlbauer commented Sep 1, 2020 • edited

djhoese commented Sep 1, 2020

djhoese commented Jun 9, 2021

macrocosme commented Nov 19, 2017 •

edited

macrocosme May 7, 2018 •

edited

macrocosme commented Jul 5, 2018 •

edited

kmuehlbauer commented Sep 1, 2020 •

edited