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] Automated and better summaries #5308

Merged
merged 3 commits into from
May 7, 2021
Merged

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Mar 4, 2021

Issue

Ref: #5108

Requires: biolab/orange-widget-base#136.

Description of changes

See biolab/orange-widget-base#136.

Includes
  • Code changes
  • Tests
  • Documentation

@janezd janezd changed the title [RFC] Automated output summary [RFC] Automated summaries Mar 4, 2021
@@ -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)
Copy link
Contributor Author

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.

Copy link
Member

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

@janezd janezd force-pushed the auto-summary branch 2 times, most recently from 087a2ae to e2d90dc Compare March 6, 2021 20:18
@janezd janezd force-pushed the auto-summary branch 2 times, most recently from 3fbbfe9 to ef9fc8f Compare March 7, 2021 10:06
@janezd janezd added the needs discussion Core developers need to discuss the issue label Mar 11, 2021
@janezd janezd changed the title [RFC] Automated summaries [ENH] Automated and better summaries Mar 11, 2021
@janezd janezd force-pushed the auto-summary branch 2 times, most recently from 7365d18 to 514f857 Compare March 12, 2021 10:06
@janezd janezd removed the needs discussion Core developers need to discuss the issue label Mar 12, 2021
@irgolic
Copy link
Member

irgolic commented Mar 12, 2021

/rebase

@janezd
Copy link
Contributor Author

janezd commented Mar 12, 2021

Rebase alone won't help. This requires a new release of orange-widget-base because of from orangewidget.utils.signals import summarize, PartialSummary. :(

@janezd janezd added the waiting for new dependency version Depends on unreleased orange-canvas-core/orange-widget-base content label Mar 12, 2021
@@ -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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.)

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

@PrimozGodec
Copy link
Contributor

widget-base is now released.

@janezd janezd force-pushed the auto-summary branch 2 times, most recently from 23e80da to 7954dd5 Compare April 30, 2021 14:01
@codecov
Copy link

codecov bot commented Apr 30, 2021

Codecov Report

Merging #5308 (a2c278b) into master (ccea9ca) will decrease coverage by 0.21%.
The diff coverage is 97.91%.

@@            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     

@janezd
Copy link
Contributor Author

janezd commented Apr 30, 2021

@VesnaT, I think this is ready (rebased, corrected and linted).

@janezd janezd removed their assignment Apr 30, 2021
@janezd janezd removed the waiting for new dependency version Depends on unreleased orange-canvas-core/orange-widget-base content label Apr 30, 2021
Copy link
Contributor

@VesnaT VesnaT left a 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, "&#9924;" if date.month == 12 else "🄼")
summarize_by_name(Learner, "🄻")
summarize_by_name(Preprocess, "🄿")
Copy link
Contributor

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?

Copy link
Contributor Author

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 \
Copy link
Contributor

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)
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They already were.

@janezd
Copy link
Contributor Author

janezd commented May 6, 2021

  • CSV File Import widget does not display the summary

And I thought I checked them all. The problem was that it used old-style output signals. I migrated it to new signals.

  • the summary code can be removed from Aggregate columns widget

This widget was moved from prototypes after I prepared this PR. :) Fixed.

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.

4 participants