-
-
Notifications
You must be signed in to change notification settings - Fork 256
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
PR: Implement "Monte Carlo" brute force "RGB" colourspace volume computation. #159
Conversation
…8" definitions signatures.
…aticity_coordinates" module private attributes.
Coverage increased (+0.08%) when pulling 4ba57edd14c1faced2e8e6b84953b09b8c8b8828 on feature/volume into 70cd9f8 on develop. |
Coverage increased (+0.08%) when pulling cc3942484771ef06f181c9898701241c59ea8fca on feature/volume into 70cd9f8 on develop. |
… using "colour.random_triplet_generator" definition.
e5a638f
to
dddca70
Compare
definition. | ||
""" | ||
|
||
prng = np.random.RandomState(4) |
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 don't think the expected behavior (consistency of the random numbers) here is guaranteed across systems/versions. This SO question seem to confirm this too.
I guess as long as it doesn't make problems this is not a big issue, but using a mock random number generator might be the "more correct" solution here to avoid this from breaking in the future.
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 yeah that's true. I haven't looked at the tests code in a while. I will add a reference so that we remember if the tests brake in the future.
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.
The reference also needs to go in colour/volume/tests/tests_rgb.py
Otherwise everything looks fine to me.
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.
Yeah, I also added the mention to the definition themselves since the doctests also use np.random.RandomState
. We should be covered.
…stests using "np.random.RandomState" definition.
Closes #158.