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] OWSelectAttributes: Use input features #3299

Merged
merged 2 commits into from
Oct 26, 2018

Conversation

VesnaT
Copy link
Contributor

@VesnaT VesnaT commented Oct 9, 2018

Issue

Enable filtering dataset using selected features.
Implements #3300

Description of changes
  • Upgrade Select Columns widget to accept Features on the input (as AttributeList). One can choose between using input features and data domain features to obtain output data. New button "Use input features" is added for this purpose. When features appear on the input, the button enables. After the features have been used, the "Features" list box disables.
    The initial state is can be obtained by clicking "Reset" button.

  • Upgrade Rank widget to output Features (as AttributeList).

Includes
  • Code changes
  • Tests
  • Documentation

@VesnaT VesnaT changed the title [ENH] OWSelectAttributes: Use input features [WIP][ENH] OWSelectAttributes: Use input features Oct 9, 2018
@VesnaT VesnaT force-pushed the sel_cols_by_features branch from a692e48 to 54c2ce7 Compare October 9, 2018 13:10
@codecov-io
Copy link

codecov-io commented Oct 9, 2018

Codecov Report

Merging #3299 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #3299      +/-   ##
=========================================
+ Coverage   82.12%   82.2%   +0.07%     
=========================================
  Files         351     351              
  Lines       62122   62334     +212     
=========================================
+ Hits        51017   51240     +223     
+ Misses      11105   11094      -11

@VesnaT VesnaT changed the title [WIP][ENH] OWSelectAttributes: Use input features [ENH] OWSelectAttributes: Use input features Oct 9, 2018
@VesnaT VesnaT force-pushed the sel_cols_by_features branch from 54c2ce7 to 6a8386b Compare October 10, 2018 09:12
Copy link
Contributor

@janezd janezd left a comment

Choose a reason for hiding this comment

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

Works as intended, except for one thing: the push button disables the list view, so the only way to get out of this is to use the Reset button. Only the checkbox should disable the view. See the code I propose below.

I like it that the button is enabled exactly when it would actually do something.

I don't like the GUI we decided upon: the checkbox and the push button now dominate the widget. As much as I prefer disabling instead of hiding, I lean towards disabling it when input features are not present.

The remaining comments I wrote are just nitpicking. Use or ignore at will.

if self.use_input_features and self.can_use_features():
self.use_features()
if not self.use_input_features:
self.enable_use_features_box()
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose I would want to have the list view disabled only when checkbox is checked:

         if self.use_input_features and self.can_use_features():
             self.use_features()
            self.enable_used_attrs(False)
        else:
            self.enable_used_attrs()

Otherwise, pushing the button disabled the list view and buttons, and the only action that gets me out of this is the Reset button.

self.use_features_box.repaint()

def use_features(self):
self.enable_used_attrs(False)
Copy link
Contributor

Choose a reason for hiding this comment

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

For the same reason as above, in __use_features_changed, I think the above line should be removed. Pushing the button should keep the list view enabled, otherwise the only option to re-enable it is the Reset button.

if feature.name in domain and domain[feature.name]
in domain.attributes]

def can_use_features(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

On the first glance, I thought that this method checks whether the features on the input are usable. I don't have a better name. Perhaps can_enable_use_features? I don't like it, but it tells me what it does.

return bool(self.features_from_data_attributes) and \
self.features_from_data_attributes != self.used_attrs[:]

def __use_features_changed(self): # Use input features check box
Copy link
Contributor

Choose a reason for hiding this comment

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

This name confuses me, too. Perhaps __on_use_features_checkbox? Not too nice, either, but I understand it.

self.Warning.mismatching_domain.clear()
if self.data is not None and self.features is not None and \
not len(self.features_from_data_attributes):
self.Warning.mismatching_domain()
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also do:

def check_data(self):
    self.Warning.mismatching_domain(
        shown=self.data is not None
        and self.features is not None
        and not self.features_from_data_attributes
    )

domain = self.data.domain
return [domain[feature.name] for feature in self.features
if feature.name in domain and domain[feature.name]
in domain.attributes]
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a matter of taste, but consider avoiding repeating domain[feature.name] by

return [var
        for var in (domain[feature.name]
                    for feature in self.features if feature.name in domain)
        if var in domain.attributes]

or

existing = (domain[feature.name] for feature in self.features
            if feature.name in domain)
return [var for var in existing if var in domain.attributes]

On the other hand, isn't this the same as

attrs = {attr.name for attr in domain.attributes}
return [domain[feature.name] if feature.name in attrs]

assuming that all names are unique?

"Use input features", "Always use input features",
box=False, commit=self.__use_features_clicked,
callback=self.__use_features_changed, addToLayout=False
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realize you can repurpose gui.auto_commit for this. I assumed there can be only a single auto_commit control per widget. :) Nice.

self.check_data()
self.enable_used_attrs()
self.enable_use_features_box()
if self.use_input_features and len(self.features_from_data_attributes):
Copy link
Contributor

Choose a reason for hiding this comment

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

Since self.features_from_data_attributes is a list, PEP8 recommends if self.use_input_features and self.features_from_data_attributes

self.move_attr_button.setEnabled(enable)
self.down_attr_button.setEnabled(enable)
self.used_attrs_view.setEnabled(enable)
self.used_attrs_view.repaint()
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that these repaints (here and in the next function) are needed? It seems to work without for me.

def enable_use_features_box(self):
self.use_features_box.button.setEnabled(self.can_use_features())
enable_checkbox = bool(self.features_from_data_attributes)
self.use_features_box.checkbox.setEnabled(enable_checkbox)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps self.use_features_box.setHidden(not enable_checkbox) (see the general comment).

@VesnaT VesnaT force-pushed the sel_cols_by_features branch 2 times, most recently from cfa7b34 to dbf75bc Compare October 26, 2018 11:53
@VesnaT VesnaT force-pushed the sel_cols_by_features branch from dbf75bc to c988589 Compare October 26, 2018 12:32
@lanzagar lanzagar merged commit 5d7e419 into biolab:master Oct 26, 2018
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