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] Create Class widget #1766

Merged
merged 8 commits into from
Dec 23, 2016
Merged

[ENH] Create Class widget #1766

merged 8 commits into from
Dec 23, 2016

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Nov 19, 2016

In line with my widely known and well established tradition of impeccable pull requests and thoroughly tested code, I am presenting a new widget that creates a class attribute with values based on substrings of the chosen string or discrete attribute. The widget is needed for infraorange and presumably also in bioinformatics.

The widget has zero pylint warnings, was assigned a straight A by radon, and has a 100 % coverage with unit tests of the highest quality. The latter rely on simulated user interaction with the widget, and restrict themselves from testing the implementational details except where appropriate. The abundance of documentation strings explaining even the smallest nuances of the private methods - but without going into unnecessary details - will make the future maintenance of the widget pure joy.

This whole grand endeavour is topped by a carefully crafted icon that complements the widget's amazing user interface.


A minor shortcoming that may need to be mentioned is that I can't make the widget properly shrink vertically when lines are removed. Not for lack of trying. Any help appreciated.

I have not written any documentation for the widget catalog so far. This will wait for any suggestions and for @ajdapretnar's help with screenshots.

The code of the widget contains two general classes derived from Transform and Lookup -- but perhaps not general enough to pollute the namespace of the transformation module.

The pull request includes a fix for Orange.preprocess.transformation.Lookup, which probably nobody used since it was plain broken. This commit could belong to another pull request, but processing/merging chains of pull requests tends to take too long in my experience. Being broken and useless, the class has two relatives of the same name in other modules, one of which does exactly the same thing as the original Lookup did not do.


@markotoplak, can you test this on files with hyperspectral data? After this glorious presentation, let's hope the widget doesn't crash on the very first use. ;)

Includes
  • Code changes
  • Tests
  • Documentation

@codecov-io
Copy link

codecov-io commented Nov 19, 2016

Current coverage is 89.18% (diff: 100%)

Merging #1766 into master will increase coverage by 0.13%

@@             master      #1766   diff @@
==========================================
  Files            85         85          
  Lines          9027       9063    +36   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           8039       8083    +44   
+ Misses          988        980     -8   
  Partials          0          0          

Sunburst

Powered by Codecov. Last update 4071837...90d4d90

@janezd janezd force-pushed the create-class branch 5 times, most recently from 66f3a0a to b398883 Compare November 23, 2016 10:50
@janezd janezd force-pushed the create-class branch 5 times, most recently from 9af2d65 to 4322195 Compare November 27, 2016 23:48
@janezd janezd changed the title [WIP] [ENH] Create Class widget [ENH] Create Class widget Nov 28, 2016
def map_by_substring(a, patterns, case_sensitive, at_beginning):
def map_by_substring(a, patterns, case_sensitive, match_beginning):
"""
Map values in a using a list of patterns. The patterns are considered in
Copy link
Contributor

Choose a reason for hiding this comment

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

Map values in a using a list of patterns.

  • What patterns?

@ajdapretnar
Copy link
Contributor

I like this widget a lot, but GUI totally confuses me. The + symbol is very small and renders as cross on Win, instance count odd, widget is a bit long... I'm not sure, it just doesn't lie well with me.
I'd make it more Select-Rows-esque, with X on the right, info box perhaps (?) and so on. And no, it's probably not the best idea either.

As for functionality, I'd like to have sth like Other, Rest or *... Something that says 'everything else belongs to this class'.

Also as for pattern, how about Regex? If not, I'd have a tooltip for 'Pattern' giving an example of use. Will be featured in docs, too.

I'd add Report and have the two bottom buttons as in Feature Constructor.

@janezd
Copy link
Contributor Author

janezd commented Nov 29, 2016

Note to self from today's discussion with @ajdapretnar:

Besides what Ajda wrote above:

  • equal width of apply and report buttons
  • narrower widget
  • align the combo with line edits
  • placeholder in the first empty pattern that tells that this will cover the remaining instances
  • tooltip on counts
  • replace the add button with normal +; compute the necessary width
  • perhaps remove the box title (Pattern)

@janezd janezd changed the title [ENH] Create Class widget [WIP] [ENH] Create Class widget Dec 2, 2016
@markotoplak
Copy link
Member

markotoplak commented Dec 9, 2016

It worked with spectral data and Files widget.

@janezd janezd changed the title [WIP] [ENH] Create Class widget [ENH] Create Class widget Dec 9, 2016
@astaric
Copy link
Member

astaric commented Dec 23, 2016

Could you also include a help file for the widget? Even if it is just a stub.

@astaric astaric merged commit b890868 into biolab:master Dec 23, 2016
@janezd janezd deleted the create-class branch April 5, 2019 17:31
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.

5 participants