-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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] Automated and better summaries #5308
Conversation
@@ -101,3 +105,8 @@ def new_line(text): | |||
details = f'No data on {type_io}.' | |||
full_details.append(details if not name else f'{name}:<br>{details}') | |||
return '<hr>'.join(full_details) | |||
|
|||
|
|||
@summarize.register(Table) |
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.
If we stop supporting Python 3.6, it would suffice to annotate the function's argument as Table
instead of passing Table
to decorator.
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.
According to NEP 29, we've been free to drop 3.6 since Jun 23, 2020. The only concern would be Anaconda (for the same reason we still tacitly support PyQt5.9).
https://numpy.org/neps/nep-0029-deprecation_policy.html
087a2ae
to
e2d90dc
Compare
3fbbfe9
to
ef9fc8f
Compare
7365d18
to
514f857
Compare
/rebase |
Rebase alone won't help. This requires a new release of orange-widget-base because of |
@@ -403,8 +402,6 @@ def __init__(self): | |||
self.__pending_selection = self.selection | |||
self._invalidated = True | |||
self._domain_invalidated = True | |||
self.input_changed.connect(self.set_input_summary) | |||
self.output_changed.connect(self.set_output_summary) |
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.
Are the signals (input_changed
and output_changed
) still needed?
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.
You tell me.
I'd prefer if they're eliminated. (Otherwise every widget could have them.)
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.
Please keep them. I know we separated the repositories, but we need the API between canvas/widget-base. I'd love to be able to read/write a widget's title from OWWidget code, too.
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.
@irgolic, this signal doesn't help you a lot. This is OWDataProjectionWidget
, not OWBaseWidget
. You want to keep it here or have it there?
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.
Sorry, I got it confused with this. https://orange-canvas-core.readthedocs.io/en/0.1.16/orangecanvas/scheme.events.html#orangecanvas.scheme.events.WorkflowEvent.OutputLinkAdded
By all means, remove these signals. :)
widget-base is now released. |
23e80da
to
7954dd5
Compare
Codecov Report
@@ Coverage Diff @@
## master #5308 +/- ##
==========================================
- Coverage 86.37% 86.15% -0.22%
==========================================
Files 303 303
Lines 62157 61592 -565
==========================================
- Hits 53688 53065 -623
- Misses 8469 8527 +58 |
@VesnaT, I think this is ready (rebased, corrected and linted). |
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.
Some observations:
CSV File Import
widget does not display the summary- the summary code can be removed from
Aggregate columns
widget
|
||
summarize_by_name(Model, "⛄" if date.month == 12 else "🄼") | ||
summarize_by_name(Learner, "🄻") | ||
summarize_by_name(Preprocess, "🄿") |
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.
Have you considered displaying all preprocessors in case of PreprocessorList
?
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.
I haven't. Now I did. Names are not nice -- they are class names. But that's all we have.
def _get_details(self): | ||
details = "Data:<br>" | ||
details += format_summary_details(self.data).replace('\n', '<br>') if \ | ||
details += format_summary_details(self.data, format=Qt.RichText) if \ |
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 if
part can now be removed.
@@ -403,8 +402,6 @@ def __init__(self): | |||
self.__pending_selection = self.selection | |||
self._invalidated = True | |||
self._domain_invalidated = True | |||
self.input_changed.connect(self.set_input_summary) |
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 signals are no longer user and can be removed.
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.
They already were.
And I thought I checked them all. The problem was that it used old-style output signals. I migrated it to new signals.
This widget was moved from prototypes after I prepared this PR. :) Fixed. |
Issue
Ref: #5108
Requires: biolab/orange-widget-base#136.
Description of changes
See biolab/orange-widget-base#136.
Includes