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

Inconsistent definitions of Data and Target classes #90

Closed
vniclas opened this issue Jun 10, 2021 · 2 comments · Fixed by #98
Closed

Inconsistent definitions of Data and Target classes #90

vniclas opened this issue Jun 10, 2021 · 2 comments · Fixed by #98
Labels
enhancement New feature or request

Comments

@vniclas
Copy link
Collaborator

vniclas commented Jun 10, 2021

Hi all,

I came across some inconsistencies between the styles used in data.py and target.py, which make the integration of new subclasses unnecessary complicated.
While data.py is rather consistent and adheres to the proposed usage of getter and setters, target.py could be improved such that the user can experience a common interface across different target classes.

In my opinion, the biggest problem is that Target does not mark its variables as private (leading underscore), which makes it cumbersome to implement proper getter/setter functions to perform type checking. I'd propose to change Target to the code below and add default setter/getters as has been done in the Data class.

class Target(BaseTarget):
    def __init__(self):
        super().__init__()
        self._data = None
        self._confidence = None
        self._action = None
@vniclas vniclas added the enhancement New feature or request label Jun 10, 2021
@vniclas vniclas mentioned this issue Jun 23, 2021
4 tasks
@passalis
Copy link
Collaborator

Hi @vniclas,

You are correct. Target should implement setters/getters as you describe. I have opened #98 where I implemented setters and getters for the base classes, as well as for a few more used by AUTH. I think this resolves the main issue (i.e., having these fields as private variables in the Target class). Let me know what you think.

@vniclas
Copy link
Collaborator Author

vniclas commented Jun 28, 2021

Hi @passalis,

thanks for looking into the issue. I directly commented on your PR.

@vniclas vniclas linked a pull request Jul 5, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants