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

replace staticmethod with classmethod in get_dataset_splits() #291

Open
mariosasko opened this issue Feb 9, 2021 · 0 comments
Open

replace staticmethod with classmethod in get_dataset_splits() #291

mariosasko opened this issue Feb 9, 2021 · 0 comments
Labels
dataset Issues or pull requests related to datasets
Milestone

Comments

@mariosasko
Copy link
Collaborator

Currently, get_dataset_splits() in our datasets is a static method (@staticmethod), but it would be more appropriate to have it marked as a class method (@classmethod).

The following example shows the difference between these two:

class A:
    @staticmethod
    def x():
         A.y()

    @staticmethod
    def y():
        print('Ay')

class B(A):
    @staticmethod
    def y():
        print('By')

B.x() prints Ay

class A:
    @classmethod
    def x(cls):
         cls.y()

    @staticmethod
    def y():
        print('Ay')

class B(A):
    @staticmethod
    def y():
        print('By')

B.x() prints By

In our datasets, x corresponds to get_dataset_splits and y corresponds to get_default_fields. The issue is that our current pattern doesn't support overriding get_default_fields by a subclass as can be seen in the example above (replace print with return fields), without overriding get_dataset_splits as well. We have this scenario in the SNLI dataset so this is the main reason why I'm opening this issue.

@mariosasko mariosasko added the dataset Issues or pull requests related to datasets label Feb 9, 2021
@mariosasko mariosasko added this to the 1.1.0 milestone Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dataset Issues or pull requests related to datasets
Projects
None yet
Development

No branches or pull requests

1 participant