[ENH] Learner widgets: Inform about potential problems when overriding preprocessors

[janezd]

Issue

Resolves #5703. Fixes #5577.

Description of changes

• Inform when preprocessors are given and the learner has its own default preprocessors, which are thusly overriden.
• Fix invalidation of model and learner, calling of updates and outputting signals.

Missing coverage is mostly in stack learner and was already missed before. This PR won't add tests there.

Includes

• Code changes
• Tests

[codecov]

Codecov Report

Merging #5710 (e44754d) into master (3408e32) will increase coverage by 0.04%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master    #5710      +/-   ##
==========================================
+ Coverage   86.11%   86.16%   +0.04%     
==========================================
  Files         315      315              
  Lines       66093    66214     +121     
==========================================
+ Hits        56918    57050     +132     
+ Misses       9175     9164      -11     

[markotoplak]

I think the message is not correct for current Orange's behavior. I like what the message implies, so perhaps we should change what Orange does.

Look, from the edited file:

    def update_learner(self):
        self.learner = self.create_learner()
        if self.learner and issubclass(self.LEARNER, Fitter):
            self.learner.use_default_preprocessors = True

create_learner() passes the given preprocessors into the learner, but in Learner we have:

    @property
    def active_preprocessors(self):
        yield from self.preprocessors
        if (self.use_default_preprocessors and
                self.preprocessors is not type(self).preprocessors):
            yield from type(self).preprocessors

If I understand this code, the default preprocessors will still be used for Fitters, the explicitly input ones are only meant to be used before the fixed ones.

Logistic regression in Orange is not a Fitter so in this specific case the message is correct, but is wrong for Fitters.

Why are Fitters (learners that can do both classification or regression) handled differently? No idea.

[markotoplak]

Perhaps we can make a different message for Fitters for now and then think about changing the behavior separately?

I have no idea what would that kind of a behavior change mean for backward compatibility, but it could get nasty. :D

[janezd]

This should have called unconditional_apply. Now the base class defines handleNewSignals and handles them properly (I suppose).

[janezd]

This was incorrect because it didn't set dirty flag, but was necessary due to broken signal handling.

[janezd]

This is horrendous, but I see no way around it (while maintaining compatibility).

This class and derived classes used apply to update and send both models; it would be better to update just the invalidated. Furthermore _change_name sent outputs by itself if auto_apply was True, but of course could not set the dirty flag if it was False. In some cases, this caused outputing the model twice...

To clean the mess, yet keep the compatilbility, I had a solution in which apply got an additional argument with default value. Unfortunatelly, gui.auto_apply doesn't pass extra arguments to apply -- and we shouldn't modify it to, just to fix this specific case.

To make it even more tricky, the function as defined here is called only when auto apply is on.

Hence, apply_as passes an argument by appending the "urgency" to the list. When the apply function is actually called, it finds the maximal "urgency" in the list and acts on it.

[janezd]

This test would fail on master because learner would be sent twice.