Skip to content

silx.gui.plot.PlotWidget: Added yaxis arcsinh support with matplotlib backend #4306

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

EdgarGF93
Copy link
Contributor

Checklist:


@vallsv
Copy link
Contributor

vallsv commented Apr 17, 2025

Nice.

This could also be a good opportunity to rework the backend API with normalization. Because actually, to set linear, we have to set setLog(False).

@EdgarGF93
Copy link
Contributor Author

EdgarGF93 commented Apr 17, 2025

I think I'll leave the opengl for somebody else :)

@t20100 t20100 changed the title yaxis arcsinh with matplotlib silx.gui.plot.PlotWidget: Added yaxis arcsinh support with matplotlib backend Apr 17, 2025
Copy link
Member

@t20100 t20100 left a comment

Choose a reason for hiding this comment

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

Let's OpenGL support for later.

I also propose to do this PR on adding arcsinh to PlotWidget with matplotlib and keep the QAction for a second one.

Could you add arcsinh scale for x axis as well for consistency?

@@ -1596,6 +1596,9 @@ def setYAxisLogarithmic(self, flag):
self._plotFrame.yAxis.isLog = flag
self._plotFrame.y2Axis.isLog = flag

def setYAxisArcsinh(self, flag):
...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
...
raise NotImplementedError("Plot OpenGL backend does not support arcsinh Y axis")

</g>
<g id="logGroup">
</g>
<text id="asinhText" x="8.0" y="19" font-weight="bold" font-family="sans-serif" font-size="0.56em">asinh</text>
Copy link
Member

Choose a reason for hiding this comment

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

Could you convert the text to a path so the icon does not rely on system fonts?
If you don't know how to do it, i can show you.

Comment on lines +1324 to +1333
def setYAxisArcsinh(self, flag):
if flag:
self.ax2.set_yscale("asinh")
self.ax.set_yscale("asinh")
return

self.ax2.set_yscale("linear")
self.ax2.yaxis.set_major_formatter(DefaultTickFormatter())
self.ax.set_yscale("linear")
self.ax.yaxis.set_major_formatter(DefaultTickFormatter())
Copy link
Member

Choose a reason for hiding this comment

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

Would be best to change the backend to have a single place where to set scale for yaxis.
I would replace setYAxisLogarithmic and setYAxisArcsinh by a setScale.

The backend are not marked as private with an _ but they should have and we don't keep the compatibility.

Comment on lines +458 to +459
def _internalSetArcsinh(self, flag):
self._getBackend().setYAxisArcsinh(flag)
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, _internalSetLogarithmic and _internalSetArcsinh could be replaced by a _internalSetScale

Comment on lines +186 to +190
self.yAxisArcsinhAction = self.group.addAction(
actions.control.YAxisArcsinhAction(self, parent=self)
)
self.yAxisArcsinhAction.setVisible(logScale)
self.addAction(self.yAxisArcsinhAction)
Copy link
Member

Choose a reason for hiding this comment

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

Adding a new axis scaling changes its control from a boolean to an enum (it should have been so since the beginning).
It would make sense to me to replace yAxisLogarithmicAction with a toolbutton with a drop-down.

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.

3 participants