-
Notifications
You must be signed in to change notification settings - Fork 17
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
Feat set seed #929
Feat set seed #929
Conversation
Draft fuinction that worked on my initial testing using OpSo 0.9.1, but still needs testing on 0.10.1
opensoundscape/seed.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can go in opensoundscape/utils.py rather than being a separate module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, but Let the unit test separate because I didn't want to mess-up test_utils.py.
tests/test_seed.py
Outdated
|
||
import pytest | ||
|
||
pytestmark = pytest.mark.parametrize("input", [1, 11, 13, 42, 59, 666, 1234]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a global version of the @pytest.mark.parametrize decorator to use the list as inputs for all functions in the script. See https://docs.pytest.org/en/7.3.x/how-to/parametrize.html for more explanation.
opensoundscape/seed.py
Outdated
import random | ||
|
||
|
||
def seed(seed, verbose=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a docstring, with Args and Return fields; see other functions for examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
opensoundscape/utils.py
Outdated
@@ -329,3 +330,25 @@ def generate_opacity_colormaps( | |||
colormaps.append(cmap) | |||
|
|||
return colormaps | |||
|
|||
|
|||
import numpy as np |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imports should always be at the top of a file. VS Code will help detect duplicate imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, missed that. Just pushed the correction.
model_resnet3 = cnn_architectures.resnet18(num_classes=10, weights=None) | ||
lw3 = model_resnet3.layer1[0].conv1.weight | ||
|
||
assert torch.all(lw1 == lw2) & torch.any(lw1 != lw3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't understand the logic here, can you add a comment? is the any
asserting that none of the weights should be the same? That doesn't seem like the correct logic, rather we want to assert that the entire array isn't equal to the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In summary, also assert that at least one element is different if different seeds. Added as a comment in tests/test_utils.py.
Feature to set random state for reproducible training. Solving #576