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

Use class setter properties if present #4682

Merged
merged 5 commits into from
May 19, 2022
Merged

Use class setter properties if present #4682

merged 5 commits into from
May 19, 2022

Conversation

adrianomartinelli
Copy link
Contributor

Objective

Add support of class properties when setting attributes. This pull request is based on this discussion with @rusty1s .

Problem

If Data is subclassed all attributes are set with __setattr__ method. This prevents users from implementing validation logic for attributes (like ensuring a certain dtype). In the example below, the developer would like to ensure that my_attr can only hold int values. This is currently not possible with the implemented __setattr__ method.

  class Graph(Data):
      def __init__(self):
          super().__init__()
          self.my_attr = 1.0
          self.my_attr2 = 2

      @property
      def my_attr(self):
          return self._my_attr

      @my_attr.setter
      def my_attr(self, value):
          self._my_attr = int(value)

Solution

The proposed solution checks if the class implements a setter method for the attribute or defaults to the standard behaviour

    def __setattr__(self, key: str, value: Any):
        propobj = getattr(self.__class__, key, None)
        if propobj is None or propobj.fset is None:
            setattr(self._store, key, value)
        else:
            propobj.fset(self, value)

This implementation is tested with test_data_subclass_setter in test/data/test_data.py. Coverage of data.py is 89%

Best,
Adriano

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #4682 (3321a2e) into master (6cb12ea) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4682   +/-   ##
=======================================
  Coverage   82.88%   82.88%           
=======================================
  Files         318      318           
  Lines       16817    16820    +3     
=======================================
+ Hits        13939    13942    +3     
  Misses       2878     2878           
Impacted Files Coverage Δ
torch_geometric/data/data.py 90.78% <100.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cb12ea...3321a2e. Read the comment docs.

Copy link
Member

@rusty1s rusty1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing. Thank you!

@rusty1s rusty1s merged commit 1bafcc4 into pyg-team:master May 19, 2022
@adrianomartinelli adrianomartinelli deleted the use-class-properties-in-data branch May 20, 2022 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants