Skip to content

Remove tests for input/output summaries#667

Merged
VesnaT merged 2 commits intobiolab:masterfrom
PrimozGodec:summary
Jun 9, 2021
Merged

Remove tests for input/output summaries#667
VesnaT merged 2 commits intobiolab:masterfrom
PrimozGodec:summary

Conversation

@PrimozGodec
Copy link
Collaborator

@PrimozGodec PrimozGodec commented Jun 1, 2021

Issue

Orange 3.29 already supports automated summaries but they are not available for older versions of Orange. Since orange3-text supports older versions of Orange, they must stay implemented in widgets but tests are not working for newer versions of Orange.

Description of changes

Removing tests for summaries

Discussion needed: some widgets (e.g. Preprocess) also report on the number of tokens. I suggest implementing a similar summary that is in Preprocess widget to all outputs connected to the corpus.

Includes
  • Code changes
  • Tests
  • Documentation

@PrimozGodec
Copy link
Collaborator Author

Tests should stop failing after #661 is merged

@PrimozGodec PrimozGodec changed the title Remote tests for input/output summaries Remove tests for input/output summaries Jun 2, 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.

The 'summary' code in widgets should be removed along with the tests.

A failing test is a good reminder of having useless code in a widget.

@PrimozGodec
Copy link
Collaborator Author

@VesnaT until we support Orange 3.28 we cannot remove code for summaries since they are still required for 3.28. I could change tests to test summaries on 3.28 and other versions (not sure if it is worth spending time with it). I can anyway make a test that will start failing when we raise the minimal version to 3.28.

self.Outputs.corpus.send(None)

# summary
if corpus:
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 is no longer relevant.

self.info.set_input_summary(input_numbers, input_string)

def update_output_summary(
self, cor_output_len: Optional[int], n_selected: Optional[int]
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an unused import of Optional among imports.

successful = len(embeddings) if embeddings else 0
unsuccessful = len(skipped) if skipped else 0
if unsuccessful > 0:
self.Warning.unsuccessful_embeddings()
Copy link
Contributor

Choose a reason for hiding this comment

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

The 'warning' code should not be removed.

@PrimozGodec
Copy link
Collaborator Author

Thank you for spotting those errors. It should be fixed now.

extras = (
(
f"<br/><nobr>Total tokens: {sum(map(len, corpus.tokens))}, "
f"Total types: {len(corpus.dictionary)}</nobr>"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my opinion total types are less understandable. @VesnaT what do you think bout replacing it with Number unique tokes or something similar.

@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2021

Codecov Report

Merging #667 (09a6752) into master (0f11ca3) will decrease coverage by 0.06%.
The diff coverage is 91.42%.

@@            Coverage Diff             @@
##           master     #667      +/-   ##
==========================================
- Coverage   74.01%   73.95%   -0.07%     
==========================================
  Files          71       72       +1     
  Lines        9153     9122      -31     
  Branches     1236     1234       -2     
==========================================
- Hits         6775     6746      -29     
+ Misses       2143     2142       -1     
+ Partials      235      234       -1     

@VesnaT VesnaT merged commit f49fe2d into biolab:master Jun 9, 2021
@PrimozGodec PrimozGodec deleted the summary branch June 17, 2021 10:07
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