Skip to content

Conversation

Moh-Yakoub
Copy link
Contributor

Fixes #1469

Description: I did some refactor for test_loss. I noticed that we tend to repeat the same tensors over test cases. That can be made more efficient by using pytest.fixtures.

Once the approach is approved I will unify this across all other test cases in later PRs.

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 28, 2021

@Moh-Yakoub thanks for the PR ! However, I'm not sure about the pros of this approach.
Btw, TPU CI failure is related.

@Moh-Yakoub
Copy link
Contributor Author

@Moh-Yakoub thanks for the PR ! However, I'm not sure about the pros of this approach.
Btw, TPU CI failure is related.

I will update the TPU test.
I believe using fixtures makes our testing more standardized. I feel that we shouldn't be repeating the fixtures - which is a constant value - across different test cases. also it allow us to add more tests in the future without just copying and pasting those values.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 28, 2021

I see your point, I think also we should replace const data by random data as well with fixed seed. Looking at the tests before the PR's change, they do not look that good neither, distributed tests are very mean and should be updated, IMO.
Personally, i'd prefer a common method reuse instead of fixtures... Especially those like

@pytest.fixture
def device():
    return None


@pytest.fixture
def requires_grad():
    return False

@Moh-Yakoub
Copy link
Contributor Author

I see your point, I think also we should replace const data by random data as well with fixed seed. Looking at the tests before the PR's change, they do not look that good neither, distributed tests are very mean and should be updated, IMO.
Personally, i'd prefer a common method reuse instead of fixtures... Especially those like

@pytest.fixture
def device():
    return None


@pytest.fixture
def requires_grad():
    return False

I've updated the tests to use method and fixed the missing test.

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Thanks @Moh-Yakoub

@vfdev-5 vfdev-5 merged commit e455460 into pytorch:master Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parametrize metric tests
2 participants