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

API consistency: seed vs random_seed vs sample_seed #5359

Closed
jni opened this issue Apr 27, 2021 · 11 comments · Fixed by #6258
Closed

API consistency: seed vs random_seed vs sample_seed #5359

jni opened this issue Apr 27, 2021 · 11 comments · Fixed by #6258
Labels
📜 type: API Involves API change(s)
Milestone

Comments

@jni
Copy link
Member

jni commented Apr 27, 2021

Description

See this comment from @grlee77:

it seems we were not very consistent in naming the argument seed vs. random_seed across the library (I think I also saw sample_seed in one place). Making this more consistent is something to consider separately for the 1.0 API.

For 1.0, we should make sure we pick one specific name. My vote is for random_seed=.

@jni jni added the 📜 type: API Involves API change(s) label Apr 27, 2021
@jni jni added this to the 1.0 milestone Apr 27, 2021
@tupui
Copy link
Contributor

tupui commented Apr 27, 2021

There are lot of random_state as well.

@jni
Copy link
Member Author

jni commented Apr 27, 2021

Of course there are. 😂

@jni
Copy link
Member Author

jni commented Apr 27, 2021

BUT. Maybe it should be random_state which allows passing in both integer seeds and an rng instance? That makes it more versatile. What do you think @tupui?

@tupui
Copy link
Contributor

tupui commented Apr 27, 2021

IMO in all case you should allow to pass None, int, np.random.Generator. It is what I proposed in the aforementioned PR. As for how it should be named, I don't know what people are using the most currently. I guess you know which functions are used the most and then which is the most common practice on these. It could help to make a choice.

@rfezzani
Copy link
Member

Grep gives:

  • random_seed in
    • segmentation.quickshift,
    • draw.random_shape,
  • random_state in
    • feature.draw_haar_like_feature,
    • measure.ransac,
    • future.graph.cut_normalized,
  • sample_seed in:
    • feature.BREAF
  • seed in:
    • transform.probabilistic_hough_line,
    • restoration.unwrap_phase,
    • data.binary_blobs,
    • util.random_noise.

@alexdesiqueira
Copy link
Member

@scikit-image/core I think it's time for us to vote this one 🙂
[TL;DR] I'm in favor of seed=.
I really like verbose arguments, but I'd go for seed= here for consistency. It's what a lot of other computational packages, scientific languages and stuff use — NumPy included.

It seems that SciPy uses random_state at scipy.sparse.random, though:

random_state{None, int, numpy.random.Generator, numpy.random.RandomState}, optional
If seed is None (or np.random), the numpy.random.RandomState singleton is used. If seed is an int, a new RandomState instance is used, seeded with seed. If seed is already a Generator or RandomState instance then that instance is used. This random state will be used for sampling the sparsity structure, but not necessarily for sampling the values of the structurally nonzero entries of the matrix.

Didn't look into that too much, though.

@tupui
Copy link
Contributor

tupui commented Jan 23, 2022

For new code: If seed is none or int, it should return a Generator. RandomState should only be returned when provided. And the global no.random.seed should not be supported. This is what we did in scipy.stats.qmc after lot of discussions on the topic. https://github.com/scipy/scipy/blob/main/scipy/stats/_qmc.py

@JDWarner
Copy link
Contributor

I'm happy to defer to the upstream and agree with @alexdesiqueira that it seems like seed= is the most common approach in NumPy/SciPy. I also like the conciseness.

@rfezzani
Copy link
Member

I also vote for seed as it is already majority in our API (please see my last #5359 (comment)).

@grlee77
Copy link
Contributor

grlee77 commented Jan 24, 2022

I agree that seed seems best given the self-consistency and NumPy consistency.

@mkcor
Copy link
Member

mkcor commented Jan 24, 2022

I'll add my voice to seed=.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 type: API Involves API change(s)
Projects
skimage2 API
In progress
Development

Successfully merging a pull request may close this issue.

9 participants