Label get param #306

BalzaniEdoardo · 2025-02-10T15:54:28Z

This PR addresses the following

Add a __getitem__ to Basis. Labels can be used to get the desired basis in the tree.
Improved label logic:
- User provided labels in composite basis must be unique.
- Default labels in composite bases (class name) is disambiguated by appending _<number>.
- Labeling a basis using another basis type class name results in an error.
get_params and set_params override default behavior: instead of reflecting the full attribute structure, basis1__basis2...__parameter_name, the parameter key is set to <label>_parameter_name.

For a concrete example, refer to the new how-to note: handling_composite_bases.md

Key Implementation: Setter of Basis

For the PR review, focus on the behavior of @basis1.setter and @basis2.setter. This property determines how the set_params is finalized. In particular, in case one uses set_params to set a basis funciton, some extra attribute must be modfied:

If one sets a basis with set_params, the new gets the _parent attribute from the current.
Same thing for input shape
And try to also grab the current label, if possible.

…e also enforced at basis initialization

billbrod

I played around with this a bit locally, and I think it behaves as expected.

In addition to the comments:

We need some linting to happen on the notebooks: we're getting a lot of changes that I think are just coming from differences in how editor vs. jupyter lab is saving them, which is distracting
I want to draw particular attention to my comment in basis/__init__.py on __all__, something is weird there
I don't understand what's going on with sklearn_get_params / get_params and with _map_parameters / remap_parameters -- can you explain? Basically, I think I want a description that's a bit more detailed than the high-level one in the PR now.
We only support __getitem__ for labels, not __getattr__, which is fine, but do we want to raise a more specific error message if someone tries to grab an attribute using something we know is a label?
Relatedly, if they use attributes, they need to do basis1.basis2.etc but getitem allows for labels. Probably worth explaining that in the new doc page?

docs/conf.py

billbrod · 2025-02-19T17:05:29Z

docs/how_to_guide/handling_composite_bases.md

+:::
+
+
+Note that if you don't provide a label, basis class name is used to construct the keys.


From here to the end belongs in the second section, I think. it's not about sklearn

I tried to move the content, let me know

docs/how_to_guide/handling_composite_bases.md

docs/how_to_guide/plot_02_glm_demo.md

docs/how_to_guide/plot_03_population_glm.md

billbrod · 2025-02-19T17:22:55Z

src/nemos/basis/_basis.py

+
+        Parameters
+        ----------
+        deep


missing description

now it is described

src/nemos/basis/_basis.py

src/nemos/basis/_basis_mixin.py

src/nemos/basis/_basis.py

…into label_get_param

BalzaniEdoardo · 2025-02-21T21:23:29Z

From last version of the PR, many things got moved around after talking with @billbrod.

Composite basis tree-structure traversing

Many operation on composite basis require updating child bases. So far, most of the machinery relied on the child basis methods and properties. This is included the label setter property implemented in this PR.

With this revised PR, I moved all the label editing logic, as well as other functions that operate over the whole basis tree, in a dedicated module. This functions are now independent from the child bases internal machinery. This is independence is not tested here but will be the object of a dedicated PR in which I will add a mock basis class with a single method, the compute_features, and make sure that I can successfully add it to nemos basis and that the overall machinery keeps working.

Get Params Logic

The overwritten get_params set_params logic has been moved to the CompositeBasis, which is the only basis that needs special machinery. Atomic basis rely on the standard sklearn machinery.

Implementation details

__sklearn_get_params__: implements the standard sklearn parameter extraction recursion which relies on class signature. Needed for Base.set_params to work correctly, otherwise signature names and key structure of the parameter dict generated would mismatch.
_get_params_and_key_map recursion similar to __sklearn_get_params__ but that re-labels the key of the parameter dictionary using basis.label, and additionally stores a key_map dictionary mapping the new keys to the standard structure based ones.
_map_params: this one calls _get_params_and_key_map and then drops the label from the key of the parameters of self (which is the top level bases), returns two dictionaries: the one storing the parameter and the one with the key mapping.
get_params: calls _map_params and return only the dictionaries with the parameters.
remap_params: decorator that wraps composite basis set_params. pre-processing It receives a parameter dictionary with either the keys based on attributes (if set_params is called internally by sklearn or Base) or the re-mapped dictionary that uses labels, and maps it back to the standard sklearn attribute structure-based dictionary. processing: calls the nemos.Base set_params on the re-formatted dictionary. post-processing:
re-assign labels.

Co-authored-by: William F. Broderick <billbrod@gmail.com>

…into label_get_param

BalzaniEdoardo

worked on the revisions! ready for a second round

src/nemos/basis/_basis.py

BalzaniEdoardo · 2025-02-21T20:21:07Z

docs/how_to_guide/handling_composite_bases.md

+:::
+
+
+Note that if you don't provide a label, basis class name is used to construct the keys.


I tried to move the content, let me know

BalzaniEdoardo · 2025-02-21T20:32:30Z

src/nemos/convolve.py

@@ -90,6 +90,8 @@ def _shift_time_axis_and_convolve(array: NDArray, eval_basis: NDArray, axis: int
    -----
    This function supports arrays of any dimensionality greater or equal than 1.
    """
+    # convert axis
+    axis = axis if axis >= 0 else array.ndim + axis


i thought I already merged this, unsure why it shows. I would keep it since it is a bugfix

BalzaniEdoardo · 2025-02-21T20:32:47Z

src/nemos/utils.py

@@ -156,6 +156,8 @@ def _pad_dimension(
        "acausal": ((pad_size) // 2, pad_size - (pad_size) // 2),
        "anti-causal": (0, pad_size),
    }
+    # convert negative axis in jax jit compilable way
+    axis = axis * (axis >= 0) + (array.ndim + axis) * (axis < 0)


same thing, I thought i merged this in main

BalzaniEdoardo · 2025-02-21T20:37:20Z

src/nemos/utils.py

-
-    all_params = obj.get_params(deep=False)
+    # use special method for basis
+    get_params = getattr(obj, "__sklearn_get_params__", obj.get_params)


I explained above, but since it is tricky I add another explenation. the idea is that we need to be able to both retrieve the actual attribute structure and the re-mapping using labels.

Why is that? Because scikit-learn (and our base class) use recursively the signature of classes implementing get/set parameters to determine which parameter can be edited. To work correctly, the recursion must be able to retrieve a dictionary with the actual structure ("basis1__basis2__..."), while the user facing get_prams should use the labels.

BalzaniEdoardo · 2025-02-21T20:41:04Z

src/nemos/utils.py

-
-    all_params = obj.get_params(deep=False)
+    # use special method for basis
+    get_params = getattr(obj, "__sklearn_get_params__", obj.get_params)


Btw here we don't need to use the private one, so i changed that, but we still need the method

src/nemos/basis/_basis_mixin.py

BalzaniEdoardo · 2025-02-21T20:50:08Z

src/nemos/basis/_basis.py

+
+        Parameters
+        ----------
+        deep


now it is described

src/nemos/basis/_basis.py

BalzaniEdoardo added 30 commits February 2, 2025 10:45

rename params

a7170e7

rename params

1ba2d73

added setter for label, added list all labels

5770d19

fix autogenerated

feca6a4

added label to composite basis

0818cba

fix label logic

842cfc5

added getitem

abfc2d3

added getitem to transformer

04bd322

set_params return self

cf28db0

improved regex

66fb783

fix setter

df21547

split method of basis in two

d17a1c3

linted

d30f0be

improved var names

4b39526

simplified func

cdc1b46

fixed behavior

ffd2890

use f string

c50d4e3

improved exception

5dfc988

renamed method

c73706a

edited tutorials

ace344d

added new note on composition?

6cfc6a1

added a card

c9d2f6a

added exception

f02a920

Merge branch 'main' into label_get_param

cd1af4c

restore original label if error is raised

64e2280

use generator to getitem

1097896

simplified search

59ed6c2

improved generate keys

02e55f5

refined map

1e14d2d

improved docstrig

655d912

BalzaniEdoardo and others added 2 commits February 18, 2025 12:36

check that the label is a string

b78d87b

update test_basis_protected_name to make sure that protected names ar…

e920443

…e also enforced at basis initialization

billbrod requested changes Feb 19, 2025

View reviewed changes

BalzaniEdoardo added 20 commits February 19, 2025 14:52

moved stuff around and linted

b010859

Merge branch 'label_get_param' of github.com:flatironinstitute/nemos …

f4cbdae

…into label_get_param

removed isinstance. added a couple of easy fixes for transfomerbasis

6f74e4d

generalized recursion

bbcb559

linted and fix clone

0b857e3

moved class-independent logic to composition_utils

9e9568c

improved iterator logic

dfbbfec

linted

a60e15c

fixed typing

5629a33

simplified endswith

77cb733

added link to identifiability

49f9548

added specific error msg

9fe879a

Merge branch 'development' into label_get_param

29f3b5d

fixed note

f4af4aa

add sklearn explenation

8ea8e9a

improve error msg for transformer basis

14188d9

removed private in repr

5f3ac67

change to indexerror

cd5651c

fix test transformer basis

192a28b

improved error msg

efa4fa0

BalzaniEdoardo and others added 5 commits February 21, 2025 16:25

linted

e4cf3b0

Update docs/how_to_guide/plot_03_population_glm.md

4e4ad7c

Co-authored-by: William F. Broderick <billbrod@gmail.com>

Update docs/how_to_guide/plot_02_glm_demo.md

1e2f698

Co-authored-by: William F. Broderick <billbrod@gmail.com>

fix docs

956d190

Merge branch 'label_get_param' of github.com:flatironinstitute/nemos …

6d18346

…into label_get_param

BalzaniEdoardo commented Feb 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Label get param #306

Label get param #306

BalzaniEdoardo commented Feb 10, 2025 •

edited

Loading

billbrod left a comment •

edited

Loading

billbrod Feb 19, 2025

BalzaniEdoardo Feb 21, 2025

billbrod Feb 19, 2025

BalzaniEdoardo Feb 21, 2025

BalzaniEdoardo commented Feb 21, 2025

BalzaniEdoardo left a comment

BalzaniEdoardo Feb 21, 2025

BalzaniEdoardo Feb 21, 2025

BalzaniEdoardo Feb 21, 2025

BalzaniEdoardo Feb 21, 2025

BalzaniEdoardo Feb 21, 2025

BalzaniEdoardo Feb 21, 2025

		:::


		Note that if you don't provide a label, basis class name is used to construct the keys.

Label get param #306

Are you sure you want to change the base?

Label get param #306

Conversation

BalzaniEdoardo commented Feb 10, 2025 • edited Loading

Key Implementation: Setter of Basis

billbrod left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BalzaniEdoardo commented Feb 21, 2025

Composite basis tree-structure traversing

Get Params Logic

Implementation details

BalzaniEdoardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BalzaniEdoardo commented Feb 10, 2025 •

edited

Loading

billbrod left a comment •

edited

Loading