-
Notifications
You must be signed in to change notification settings - Fork 80
silx.gui.plot: Improved StatsWidget display slightly, especially for 1D plots #4361
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
Conversation
This patch simply deals with the common case of 1D plots (therefore with 1 independent dimension), which before these changes, would be rendered as a tuple of numpy values, and now is properly rendered as a single value. Signed-off-by: Sofia Donato Ferreira <[email protected]>
… view Signed-off-by: Sofia Donato Ferreira <[email protected]>
Signed-off-by: Sofia Donato Ferreira <[email protected]>
payno
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for submitting your pull request 🎉 .
Sorry for the late review
The two last commits (regarding Table formatting) seems of general interest 🤩.
However the first one appears to be too specific to 1D data.
Could you please revert it (or remove it) so we can merge it ?
| for resName, resValue in res.items(): | ||
| if isinstance(resValue, tuple) and len(resValue) == 1: | ||
| resValue = resValue[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with this approach is that we won't be able to display tuples as results of stats. But we need it.
For example with 2D or 3D data, we expect the StatCoordMin method to return a tuple of values—and that’s precisely what we want to show to the user.
The simplest alternative is probably to create you own StatCoordMin that would return a scalar instead of a tuple like
class MyStatCoordMin(StatCoordMin):
def calculate(self, context):
result = super().calculate(context)
return result[0]There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I added the check len(resValue) == 1, so that it is specific to 1D values. Though in 2D+ cases, perhaps the best approach would be to change the format method to also check if the value is a tuple or a list even when self.formatters[name] is None, so we can call str on each individual element anyway, and get rid of the np.floatXY text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The StatsHandler.calculate function only delegates the calculation of the result.
Similarly, StatsHandler.format simply delegates to the formatter associated with the Stats object. This way we maintain a certain separation of concerns.
Given the current design, I think there are two approaches to address your use case:
-
Use the default formatter and ensure your stats return a scalar value, as demonstrated in the
MyStatCoordMinexample. -
Provide a custom formatter as you suggested. This approach might be more involved. An example is given in examples/plotStats.py. And you can define the stats to be computed with it (formatter)[https://docs.python.org/3.9/library/formatter.html].
For example if I resume the example withStatCoordMinyou would have something like
plot = Plot2D()
stats = [
(StatCoordMin(), "{0:.2f}"),
]
plot.getStatsWidget().setStats(stats)| for column, tableItem in enumerate(tableItems): | ||
| tableItem.setData(qt.Qt.UserRole, _Container(item)) | ||
| tableItem.setFlags(qt.Qt.ItemIsEnabled | qt.Qt.ItemIsSelectable) | ||
| tableItem.setTextAlignment(qt.Qt.AlignHCenter | qt.Qt.AlignVCenter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| horizontalHeader = self.horizontalHeader() | ||
| horizontalHeader.setSectionResizeMode(qt.QHeaderView.ResizeToContents) | ||
|
|
||
| horizontalHeader.setStretchLastSection(True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
@flowln I created a new PR #4445 that includes your modifications that are of general interest. 9cce125 will not be merged for the reasons explained upped. If you have any questions or would like to discuss the omitted parts, feel free to reply here or open a new issue. We appreciate the effort you put into this work and hope you’ll continue to contribute! |
|
Oh, I was going to do just that, but got caught up with other work hahah. Thank you for the effort @payno! |
This PR does a few small improvements to the way we display statistics in
StatsWidget, especially in the case of 1D plots. I've made the smallest amount of changes possible, but any suggestions of better places to do changes are welcome!Before PR, on a sample 1D plot:
After PR, on the same sample and plot: