Transformer class for the property selection #137

hurricane642 · 2022-11-24T11:04:05Z

Hello, everyone!
This PR is the first step in adding the 'Transformer' class to rascaline. The 'Transformer' class makes it easy to create a representation matrix when using some other matrix as a reference. A classic use case is to create a TensorMap representation for a dataset, then perform transformations within that TensorMap (e.g., keys_to_features or keys_to_properties), and select the most useful features in the transformed TensorMap. The 'Transformer' allows a set of these features to be used to calculate a new TensorMap, thus saving computation time and maintaining a single I tried to explain the logic of the algorithm in the comments to the code, but if you have any questions - ask, I'll be happy to answer, as well as happy to receive any comments and suggestions.

IMPORTANT:
We need to add to the code the ability to perform several transformations one by one. This will be the next step.
Note that this branch is based on the selected_keys branch, which itself is still a PR. representation for all representations.

github-actions · 2022-11-24T11:09:17Z

Here is a pre-built version of the code in this pull request: wheels.zip, you can install it locally by unzipping wheels.zip and using pip to install the file matching your system

github-actions · 2022-11-24T11:16:04Z

The documentation for this PR is (or will soon be) available on readthedocs: https://rascaline--137.org.readthedocs.build/en/137/

ceriottm · 2022-12-06T14:49:11Z

I noticed the branch before seeing the PR and was wondering what this was. I think that transformer has too much baggage in ML to use it as a class that enacts a generic transformation of a tensor. We need a better name.

hurricane642 · 2022-12-06T19:30:17Z

I noticed the branch before seeing the PR and was wondering what this was. I think that transformer has too much baggage in ML to use it as a class that enacts a generic transformation of a tensor. We need a better name.

Yes, of course the name will be changed, it's a draft version of the name

Luthaf

the code is not very clear to me, so let's try to iterate a bit on the attributes/functions naming 😃

Luthaf · 2022-12-08T11:15:12Z

python/rascaline/transformer.py

+from equistore import Labels, TensorBlock, TensorMap
+
+
+class Transformer:


how about something like this? Then explaining in the docstring why it is useful.

Suggested change

class Transformer:

class PropertiesSelector:

Yep, sounds great!

Luthaf · 2022-12-08T11:16:49Z

python/rascaline/transformer.py

+        This variable can accept knowledge of type str (one key), list (a list
+        of keys) and Labels (in addition to the name of the keys, pass a list
+        of keys to be moved).


Suggested change

This variable can accept knowledge of type str (one key), list (a list

of keys) and Labels (in addition to the name of the keys, pass a list

of keys to be moved).

This variable can be anything supported by the :py:class:`equistore.TensorMap.keys_to_properties`

or :py:class:`equistore.TensorMap.keys_to_samples` functions, i.e. one string, a list of strings or an

instance of :py:class:`equistore.Labels`

Luthaf · 2022-12-08T11:17:34Z

python/rascaline/transformer.py

+        self.selector = selector
+        self.transformation = transformation
+        self.moved_keys = moved_keys


The user should not access these after creating an instance of this class

Suggested change

self.selector = selector

self.transformation = transformation

self.moved_keys = moved_keys

self._selector = selector

self._transformation = transformation

self._moved_keys = moved_keys

Luthaf · 2022-12-08T11:18:10Z

python/rascaline/transformer.py

+        if (self.transformation is None) and (self.moved_keys is not None):
+            raise ValueError("unable to shift keys: unknown transformation type")
+
+    def _copy(self, tensor_map):


why do you need to make a copy?

In the _mover function, I move keys_to_samples/keys_to_properties with the tensor, saving its keys before doing so. Before I used the _copy function, the saved keys started pointing to the keys of the already modified tensor.

Right. If you only need the keys, it might be better to only copy the keys, they should work fine with tensor.keys.copy()

Luthaf · 2022-12-08T11:18:18Z

python/rascaline/transformer.py

+            blocks.append(block.copy())
+        return TensorMap(tensor_map.keys, blocks)
+
+    def keys_definition(self):


Suggested change

def keys_definition(self):

def _keys_definition(self):

Luthaf · 2022-12-08T11:21:50Z

python/rascaline/transformer.py

+            self.moved_keys_names = self.moved_keys.copy()
+            self.final_keys = self._old_keys
+        else:
+            # The third case is a little more complicated.


Suggested change

# The third case is a little more complicated.

assert isinstance(self.moved_keys, Labels)

# The third case is a little more complicated.

Luthaf · 2022-12-08T11:22:16Z

python/rascaline/transformer.py

+        of keys to be moved).
+    """
+
+    def __init__(self, selector, transformation=None, moved_keys=None):


we should use the same name as the equistore functions

Suggested change

def __init__(self, selector, transformation=None, moved_keys=None):

def __init__(self, selector, transformation=None, keys_to_move=None):

Luthaf · 2022-12-08T11:23:23Z

python/tests/transformer.py

+
+import numpy as np
+from equistore import TensorBlock, TensorMap
+from skcosmo.feature_selection import FPS as FPS_f


why do you rename FPS to FPS_f?

I forgot to rename it from the time I thought it would be necessary to make a sample selection too. I'll rename it

Luthaf · 2022-12-08T11:24:57Z

python/rascaline/transformer.py

+        self.keys_definition()
+        self.properties_selection()
+
+    def transform(self, frames, calculator):


Should this class take the calculator in the constructor? Given that it will only be usable with one calculator.

And this function also needs to handle all other kwargs that could be given to the calculator (mainly gradients & use_native_system). We could either have them explicitly, or take a **kwargs argument, set the corresponding selected_properties and pass it to the calculator

On the first point, it's an interesting question. So, it is clear that we need to input frames (our general task is to get descriptor from them as simply as possible). For this, we need a calculator one way or another. And now the question is at what point we will feed it as input? We either have to do it in the fit function or already in transform. In general, the first option is possible (we can pass either descriptor and calculator or frames and calculator to fit). What do you think would be better?
On the second point, yes, I agree, I'll try to fix that.

I would pass the calculator even earlier (in __init__), and then both fit and transform take a set of frames + some options as kwargs

Luthaf · 2022-12-08T11:25:14Z

python/rascaline/transformer.py

+                tensor_copy.keys_to_properties(self.moved_keys)
+        return tensor_copy
+
+    def properties_selection(self):


Suggested change

def properties_selection(self):

def _properties_selection(self):

Luthaf · 2022-12-08T11:29:43Z

You can also add skcosmo to the tox environments to be able to run the tests.

hurricane642 · 2022-12-19T19:29:31Z

You can also add skcosmo to the tox environments to be able to run the tests.

Ok, I added it, but it's still having trouble passing the tests. Is there anything else I should add?

Luthaf · 2022-12-20T14:40:46Z

You can skip tests if skcosmo is not installed (as long as it is still tested in the all-deps environement), see for example

rascaline/python/tests/systems/chemfiles.py

Line 17 in 3a28ad0

@unittest.skipIf(not HAVE_CHEMFILES, "chemfiles is not installed")

Luthaf · 2022-12-20T14:41:21Z

setup.cfg

@@ -21,6 +21,7 @@ package_dir =
 install_requires =
    numpy
    equistore @ https://github.com/lab-cosmo/equistore/archive/6ca7fa3.zip
+    skcosmo


This should not be in the install_requires (it is an optional dependency, like ase or chemfiles)

hurricane642 requested a review from Luthaf November 24, 2022 11:04

Luthaf force-pushed the selected_keys branch 3 times, most recently from 123d726 to 4842eb9 Compare December 1, 2022 11:04

Base automatically changed from selected_keys to master December 6, 2022 13:48

add first version of the complex transformer for property selection

01d01c3

hurricane642 force-pushed the transformer branch from f685faf to 01d01c3 Compare December 6, 2022 14:21

Luthaf reviewed Dec 8, 2022

View reviewed changes

changes in naming

0865e50

Luthaf reviewed Dec 20, 2022

View reviewed changes

add check of skmatter installation

73e5d07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transformer class for the property selection #137

Transformer class for the property selection #137

hurricane642 commented Nov 24, 2022

github-actions bot commented Nov 24, 2022 •

edited

Loading

github-actions bot commented Nov 24, 2022

ceriottm commented Dec 6, 2022

hurricane642 commented Dec 6, 2022

Luthaf left a comment

Luthaf Dec 8, 2022

hurricane642 Dec 9, 2022

Luthaf Dec 8, 2022

Luthaf Dec 8, 2022

Luthaf Dec 8, 2022

hurricane642 Dec 9, 2022

Luthaf Dec 9, 2022

Luthaf Dec 8, 2022

Luthaf Dec 8, 2022

Luthaf Dec 8, 2022

Luthaf Dec 8, 2022

hurricane642 Dec 9, 2022

Luthaf Dec 8, 2022

Luthaf Dec 8, 2022

hurricane642 Dec 9, 2022

Luthaf Dec 9, 2022

Luthaf Dec 8, 2022

Luthaf commented Dec 8, 2022

hurricane642 commented Dec 19, 2022

Luthaf commented Dec 20, 2022

Luthaf Dec 20, 2022

		from equistore import Labels, TensorBlock, TensorMap


		class Transformer:

	def __init__(self, selector, transformation=None, moved_keys=None):
	def __init__(self, selector, transformation=None, keys_to_move=None):

	def properties_selection(self):
	def _properties_selection(self):

Transformer class for the property selection #137

Are you sure you want to change the base?

Transformer class for the property selection #137

Conversation

hurricane642 commented Nov 24, 2022

github-actions bot commented Nov 24, 2022 • edited Loading

github-actions bot commented Nov 24, 2022

ceriottm commented Dec 6, 2022

hurricane642 commented Dec 6, 2022

Luthaf 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

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

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

Luthaf commented Dec 8, 2022

hurricane642 commented Dec 19, 2022

Luthaf commented Dec 20, 2022

Choose a reason for hiding this comment

github-actions bot commented Nov 24, 2022 •

edited

Loading