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

[ENH] HeatMap: Allow setting the center when using diverging palettes #4809

Merged
merged 4 commits into from
Jun 1, 2020

Conversation

janezd
Copy link
Contributor

@janezd janezd commented May 24, 2020

Issue

Resolves #4789.

Includes
  • Code changes
  • Tests
  • Documentation

@janezd janezd force-pushed the set-palette-center branch from ad0f944 to 984f1e1 Compare May 24, 2020 11:18
@codecov
Copy link

codecov bot commented May 24, 2020

Codecov Report

Merging #4809 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4809      +/-   ##
==========================================
- Coverage   84.01%   84.01%   -0.01%     
==========================================
  Files         281      276       -5     
  Lines       56899    56140     -759     
==========================================
- Hits        47805    47165     -640     
+ Misses       9094     8975     -119     

@janezd janezd changed the title HeatMap: Allow setting the center when using diverging palettes [RFC] [ENH] HeatMap: Allow setting the center when using diverging palettes May 27, 2020
@Hrovatin
Copy link
Contributor

This seems to work.
However, as the centering window shows only after the diverging palette is chosen this might be hard to find for new users. Other heatmap GUI options seem to be always shown, but do not allow selection/changes if they would not make sense. Maybe the same could be done for the Center at field; e.g. grayed out if non-diverging palette is selected.
Also, other GUI options seem to be aligned more to the left, while Center at is aligned to the right. There could also be a bit more space between Center at and palette selector.

@janezd
Copy link
Contributor Author

janezd commented May 29, 2020

However, as the centering window shows only after the diverging palette is chosen this might be hard to find for new users. Other heatmap GUI options seem to be always shown, but do not allow selection/changes if they would not make sense. Maybe the same could be done for the Center at field; e.g. grayed out if non-diverging palette is selected.

I wanted to minimize the height, which is already large. The benefit of having a disabled checkbox would be to let the user know it is there, but it still wouldn't tell her how to enable it. What do you say about adding a hint, like in, say Confusion, that shows the first time the user opens the widget? Something like "For data with a 'base value', choose a diverging palette and set its center."

Also, other GUI options seem to be aligned more to the left, while Center at is aligned to the right. There could also be a bit more space between Center at and palette selector.

I wanted the opposite effect. :) I wanted to "tie" this option to the palette by aligning it to the right and putting it as close to palette as I could. If this intention is not clear, I should indeed align it to the left, like normal options.

Summary: I would keep hiding, but add a hint. I will align the checkbox to the left.

@janezd janezd force-pushed the set-palette-center branch from 984f1e1 to cb05816 Compare May 29, 2020 09:30
return self.__center

def setCenter(self, center: float) -> None:
def setCenter(self, center: Union[float, bool]) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional[float]?

Also
centerChanged = Signal(float) needs to be changed to centerChanged = Signal(object)

Copy link
Contributor Author

@janezd janezd May 29, 2020

Choose a reason for hiding this comment

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

Not really; Optional[float] would be equivalent to Union[float, None], while this was really Union[float, bool].

Terrible design. Now constructor gets either a float or None. In the latter case there is no centering, and it can't be re-enabled by changing center from None to float.

@janezd janezd force-pushed the set-palette-center branch 3 times, most recently from dae3822 to 3b40630 Compare May 29, 2020 14:11
@janezd janezd changed the title [RFC] [ENH] HeatMap: Allow setting the center when using diverging palettes [ENH] HeatMap: Allow setting the center when using diverging palettes May 29, 2020
@Hrovatin
Copy link
Contributor

If the Center at appears only for certain palettes then it is probably not that odd if it is located on the right, close to the palette selection box. (So left or right would be fine by me then.)

@janezd janezd force-pushed the set-palette-center branch from d790f5f to ff1841a Compare May 29, 2020 15:19
@janezd
Copy link
Contributor Author

janezd commented May 29, 2020

If the Center at appears only for certain palettes then it is probably not that odd if it is located on the right, close to the palette selection box. (So left or right would be fine by me then.)

OK, I prefer it on the right side. Moving back.

@ales-erjavec ales-erjavec merged commit 46932b5 into biolab:master Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heat map center value feature request
3 participants