[ENH] Parameter Fitter: Basic implementation by VesnaT · Pull Request #6921 · biolab/orange3

VesnaT · 2024-10-21T12:00:52Z

Issue

Implement a Parameter Fitter widget:

The initial version can only fit integer parameters.
to fit a parameter, the Learner has to implement a fitted_parameters method.

Includes

Code changes
Tests
Documentation

codecov · 2024-10-21T12:20:06Z

Codecov Report

Attention: Patch coverage is 95.61905% with 23 lines in your changes missing coverage. Please review.

Project coverage is 88.45%. Comparing base (52df166) to head (932fe14).
Report is 126 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6921      +/-   ##
==========================================
+ Coverage   88.37%   88.45%   +0.07%     
==========================================
  Files         329      331       +2     
  Lines       72447    73063     +616     
==========================================
+ Hits        64028    64625     +597     
- Misses       8419     8438      +19

janezd

I haven't really reviewed the PR - I haven't even checked it out from the repo. Here are just two things I incidentally noticed. Feel free to ignore.

Orange/base.py

Orange/classification/random_forest.py

Orange/widgets/evaluate/owparameterfitter.py

janezd · 2024-10-24T10:31:08Z

Orange/base.py

+            ("type", Type),
+            ("min", Union[int, NoneType]),
+            ("max", Union[int, NoneType]),
+        ]


What I had in mind was

class FittedParameter(NamedTuple): name: str label: str tick_label: str = "" type_: type = int min: Optional[int] = None max: Optional[int] = None

No need to change it; I find this easier on the eye, but it's a matter of taste.

janezd · 2024-10-25T19:18:49Z

Orange/widgets/evaluate/owparameterfitter.py

        fitted_parameter_props: Learner.FittedParameter,
-        initial_parameters: dict,
-        steps: Sized,
+        initial_parameters: dict[str, Any],


Currently, we'd have dict[str, int], but eventually we'd have dict[str, Union[int, float]]. On the other hand, what if the learner provides a list of string arguments? So I put dict[str, Any]. We can put a humble int there, though, for the time being.

I'd put an int just to emphasize that other types are not supported and should be carefully reconsidered.

Orange/widgets/evaluate/owparameterfitter.py

janezd · 2024-10-25T19:32:59Z

Orange/widgets/evaluate/owparameterfitter.py

    @property
    def initial_parameters(self) -> dict:
-        if not self._learner or not self._data:
+        if not self._learner:


Same as above.

Orange/widgets/evaluate/owparameterfitter.py

janezd · 2024-10-25T19:40:58Z

I focused on the gui layout and changed a few minor things I encountered.

In the future, we probably want to support:

optimization of multiple arguments,
non-int arguments (at least floats),
outputting a learner with internal fitting.

So, I thought about making some other changes, but decided not to, because they would probably be changed again in that future.

VesnaT

Two more things I'd suggest changing:

Steps preview does not refresh when key down/up in spin box
If I select Manual and there is no range in the line edit, the behaviour is incorrect.

VesnaT · 2024-10-29T09:53:56Z

Orange/widgets/evaluate/owparameterfitter.py

        fitted_parameter_props: Learner.FittedParameter,
-        initial_parameters: dict,
-        steps: Sized,
+        initial_parameters: dict[str, Any],


I'd put an int just to emphasize that other types are not supported and should be carefully reconsidered.

VesnaT · 2024-10-29T09:55:02Z

Orange/widgets/evaluate/owparameterfitter.py

-        initial_parameters: dict,
-        steps: Sized,
+        initial_parameters: dict[str, Any],
+        steps: Collection[Any],


Also an int

VesnaT · 2024-10-29T09:57:06Z

Orange/widgets/evaluate/owparameterfitter.py

-        initial_parameters: dict,
-        steps: Sized,
+        initial_parameters: dict[str, Any],
+        steps: Collection[Any],


Any => int

Orange/widgets/evaluate/owparameterfitter.py

VesnaT · 2024-10-29T10:23:42Z

Orange/widgets/evaluate/owparameterfitter.py

+        rect.adjust(style.pixelMetric(style.PM_IndicatorWidth)
+                    + style.pixelMetric(style.PM_CheckBoxLabelSpacing), 0, 0, 0)
+
+        last_text = f", {self.__steps[-1]}"


I'd remove the , for a single value.

janezd · 2024-10-30T14:57:17Z

I think I've addressed all comments except for

Steps preview does not refresh when key down/up in spin box

This looks like a problem in gui.spin. Open an arbitrary widget, e.g. Tree Learner and key up/down on some spin. It doesn't recompute.

markotoplak · 2024-11-05T11:27:15Z

You likely noticed this already, but this PR is not Python 3.9 compatible. Or are we planning to drop support for 3.9? Numpy already did according to https://numpy.org/neps/nep-0029-deprecation_policy.html.

… 'problem_type'

janezd · 2024-11-07T18:59:05Z

I added rudimentary documentation. The widget is bound to significantly change in the future, so it's not worth spending too much time on this.

lanzagar

The widget crashes when given a dataset without a class.
I think the controls in Parameter Fitter should be tied to the Learner input. One should be able to select a parameter to fit and the range of values before connecting the data input (since that already starts the fitting and I would like it to use the correct settings). Also, refreshing the data or even inputting a completely different dataset should not reset the settings in Parameter Fitter. I think it is acceptable if they are reset when the Learner input is refreshed, but ideally it would even try to keep the settings when e.g. I just change the name of the learner or some of the other learner parameters (maybe we don't reset to default if the class of the learner stays the same?).

Orange/widgets/evaluate/owparameterfitter.py

Orange/regression/pls.py

Orange/widgets/evaluate/owparameterfitter.py

markotoplak · 2024-11-12T07:30:03Z

Orange/widgets/evaluate/owparameterfitter.py

+    def set_learner(self, learner: Optional[Learner]):
+        if self._learner:
+            self.__initialize_settings = \
+                not isinstance(self._learner, type(learner))


If you need an exact type, do not use isinstance (this also allows for subclasses).

I would do it differently, though, by just comparing if these two leartners have the same fitted_parameters.

markotoplak changed the title ~~Parameter Fitter: Basic implementation~~ [ENH] Parameter Fitter: Basic implementation Oct 21, 2024

markotoplak added this to the 3.38.0 milestone Oct 21, 2024

VesnaT force-pushed the summary_of_fit branch from 10bd677 to 42c9de4 Compare October 21, 2024 12:45

janezd reviewed Oct 22, 2024

View reviewed changes

janezd reviewed Oct 24, 2024

View reviewed changes

janezd self-assigned this Oct 25, 2024

janezd reviewed Oct 25, 2024

View reviewed changes

janezd force-pushed the summary_of_fit branch from 6b825d3 to 4e862e5 Compare October 25, 2024 19:36

VesnaT commented Oct 29, 2024

View reviewed changes

janezd force-pushed the summary_of_fit branch 2 times, most recently from d4a1145 to ae4ee9e Compare November 4, 2024 22:24

janezd force-pushed the summary_of_fit branch 7 times, most recently from c702f4d to 0ca5f2a Compare November 6, 2024 21:13

VesnaT added 8 commits November 7, 2024 11:41

Parameter fitter: Add widget

a59288f

Parameter Fitter: Add tooltip

98b8035

Parameter Fitter: Support fitting # of trees in RF

bf35a8f

Parameter Fitter: Manual steps

8055486

Parameter Fitter: Assure some space below ticks

e38b011

Parameter Fitter: Tests

d353372

Parameter Fitter: Report

493d5b5

Parameter Fitter: Type annotations

7785104

janezd and others added 9 commits November 7, 2024 11:41

Parameter Fitter: Change icon

26f3c6f

Parameter Fitter: Fix send_report

f4791c5

Parameter Fitter: Fix minor glitches and annotations

29a2541

Parameter Fitter: Check ranges for manual edit

f9dcd02

Parameter Fitter: Slovenian translation

04d187d

Parameter Fitter: Lint

272c124

Parameter Fitter: Allow dots in sequences

e34d66d

Parameter Fitter: Replace variadic argument in fitted_parameters with…

041e242

… 'problem_type'

Parameter Fitter: Renovate definition of FitterParameter

b30ddcb

janezd force-pushed the summary_of_fit branch from 0ca5f2a to b30ddcb Compare November 7, 2024 10:41

janezd added 2 commits November 7, 2024 18:42

Parameter Fitter: Add missing translations

b3dd2eb

Parameter Fitter: Add documentation

19bdabc

lanzagar reviewed Nov 8, 2024

View reviewed changes

Orange/widgets/evaluate/owparameterfitter.py Outdated Show resolved Hide resolved

Orange/regression/pls.py Outdated Show resolved Hide resolved

VesnaT self-assigned this Nov 8, 2024

VesnaT added 5 commits November 8, 2024 15:02

FittedParameter: Remove unused property

26cefa2

PLS: Rename fitted parameter to 'Components'

d0c9c1b

Parameter Fitter: Handle classless data

15baede

Parameter Fitter: Handle multiclass data

3abcbed

Parameter Fitter: Dont silence exceptions

8376e3b

VesnaT force-pushed the summary_of_fit branch 4 times, most recently from 25bd989 to 24638c0 Compare November 12, 2024 07:05

markotoplak reviewed Nov 12, 2024

View reviewed changes

Orange/widgets/evaluate/owparameterfitter.py Show resolved Hide resolved

markotoplak reviewed Nov 12, 2024

View reviewed changes

VesnaT force-pushed the summary_of_fit branch from 24638c0 to 56b705f Compare November 12, 2024 12:58

Parameter Fitter: Retain settings on input

932fe14

VesnaT force-pushed the summary_of_fit branch from 56b705f to 932fe14 Compare November 13, 2024 14:04

lanzagar merged commit 2869a55 into biolab:master Nov 13, 2024

Uh oh!

Conversation

VesnaT commented Oct 21, 2024

Issue

Includes

Uh oh!

codecov bot commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

janezd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

janezd commented Oct 25, 2024

Uh oh!

VesnaT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janezd commented Oct 30, 2024

Uh oh!

markotoplak commented Nov 5, 2024

Uh oh!

janezd commented Nov 7, 2024

Uh oh!

lanzagar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Oct 21, 2024 •

edited

Loading