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

Remove TensorConstant.tag.unique_value #648

Closed
brandonwillard opened this issue Nov 2, 2021 · 2 comments · Fixed by #650
Closed

Remove TensorConstant.tag.unique_value #648

brandonwillard opened this issue Nov 2, 2021 · 2 comments · Fixed by #650
Labels
good first issue Good for newcomers help wanted Extra attention is needed refactor This issue involves refactoring

Comments

@brandonwillard
Copy link
Member

brandonwillard commented Nov 2, 2021

TensorConstant.tag.unique_value is an unnecessary specialized field used in only a few places. This field also overlaps with the logic in get_scalar_constant_value and extract_constant, making for yet another redundancy. Since this redundancy is attached to a core type, its removal has some priority over the other two functions.

In general, it can be easily replaced by a simple helper function—one that could also replace some of the functionality provided by get_scalar_constant_value and extract_constant, both of which are also slated for removal/replacement (see here).

@brandonwillard brandonwillard added refactor This issue involves refactoring good first issue Good for newcomers help wanted Extra attention is needed labels Nov 2, 2021
@Carlosbogo
Copy link
Contributor

I would be glad to help with this, but I think it would be very helpful to have a more in depth explanation on what the term "helper function" implies here.

@brandonwillard
Copy link
Member Author

brandonwillard commented Nov 2, 2021

Constants, and TensorConstants by extension, hold their constant values (i.e. np.ndarrays) in a Constant.data field. TensorConstant.tag.unique_value is supposed to be a scalar "reduction" of that field, so, for instance, if the field's value is a single scalar value duplicated/broadcasted in some way, TensorConstant.tag.unique_value will be that scalar value.

This logic is specified here, and it can be extracted into a simple helper function with a name like get_unique_value and used explicitly by any logic currently depending on TensorConstant.tag.unique_value.

From what I can see, these are the only references to unique_value in the project:

./aesara/scan/opt.py:169:            and node_inp.tag.unique_value is not None
./aesara/tensor/math_opt.py:135:        if getattr(v.tag, "unique_value", None) is not None:
./aesara/tensor/math_opt.py:136:            data = v.tag.unique_value
./aesara/tensor/var.py:969:        self.tag.unique_value = None
./aesara/tensor/var.py:974:                    self.tag.unique_value = flat_data[0]
./aesara/tensor/var.py:977:        if self.tag.unique_value is not None:
./aesara/tensor/var.py:978:            name = f"{self.data.shape} of {self.tag.unique_value}"
./aesara/tensor/basic.py:285:        if getattr(v.tag, "unique_value", None) is not None:
./aesara/tensor/basic.py:286:            data = v.tag.unique_value
./aesara/tensor/basic.py:299:        if getattr(v.tag, "unique_value", None) is not None:
./aesara/tensor/basic.py:300:            data = v.tag.unique_value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed refactor This issue involves refactoring
Projects
None yet
2 participants