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

Remove rng parameter of binary_blobs() function in plot_random_walker_segmentation.py #7219

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aymuos15
Copy link

Using the rng parameter throws the following TypeError:

TypeError: binary_blobs() got an unexpected keyword argument 'rng'

Description

Parameter rng removal in line:

data = skimage.img_as_float(binary_blobs(length=128, rng=1))

of Random walker segmentation Example.

## Removed rng parameter.
data = skimage.img_as_float(binary_blobs(length=128))

To reproduce error and fix: https://colab.research.google.com/drive/1iy8InDNAoadAaNkn0WuyND6S0a3VFmEx?usp=sharing

Removing it does not make any change to the aim of the tutorial; which is showing the segmentation capabilities.

Using the rng parameter throws TypeError:

TypeError: binary_blobs() got an unexpected keyword argument 'rng'
@lagru
Copy link
Member

lagru commented Oct 20, 2023

Hey @aymuos15, thanks for the suggestion. I am a bit confused why you get the TypeError. rng is a new argument that replaces the deprecated seed. Google doesn't allow me to check without sign-in, but I am assuming that your Colab notebook uses an outdated scikit-image version. So for that reason alone I wouldn't remove the parameter.

Removing it does not make any change to the aim of the tutorial; which is showing the segmentation capabilities.

I agree, though I think in general we try to pin our examples so they are completely reproduce-able. So I am hesitant to remove the rng parameter.

@lagru lagru added the 📄 type: Documentation Updates, fixes and additions to documentation label Oct 20, 2023
Copy link
Member

@soupault soupault left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, @aymuos15 ! I personally do not have a strong preference on what is better here - higher simplicity or higher reproducibility.

@aymuos15
Copy link
Author

@lagru You are right. It was a version error. I apologise for not checking before. Was a mistake on my part. Will close the request if that is okay with you. Thank you for pointing it out.

@soupault I personally feel the lesser the parameters in a base example, the better. But this is just a update issue on colab so doesn't matter that much I guess?

@lagru
Copy link
Member

lagru commented Oct 20, 2023

Ideally we could have both. It seems we don't use explicit seeds in a lot of gallery examples.

$ grep "default_rng" -r doc/examples/
doc/examples/filters/plot_inpaint.py:rstate = np.random.default_rng(0)
doc/examples/filters/plot_entropy.py:rng = np.random.default_rng()
doc/examples/filters/plot_deconvolution.py:rng = np.random.default_rng()
doc/examples/filters/plot_restoration.py:rng = np.random.default_rng()
doc/examples/features_detection/plot_shape_index.py:    rng = np.random.default_rng()
doc/examples/applications/plot_colocalization_metrics.py:rng = np.random.default_rng()
doc/examples/applications/plot_rank_filters.py:rng = np.random.default_rng()
doc/examples/registration/plot_masked_register_translation.py:rng = np.random.default_rng()
doc/examples/transform/plot_ransac.py:rng = np.random.default_rng()
doc/examples/transform/plot_ssim.py:rng = np.random.default_rng()
doc/examples/transform/plot_fundamental_matrix.py:rng = np.random.default_rng(random_seed)
doc/examples/segmentation/plot_random_walker_segmentation.py:rng = np.random.default_rng()

Most cases are for noise though. But there is precedent for both options.

In this case, I feel like for functions whose API involves randomness it's actually not a bad thing to make its usage visible. So I'm +0.1 on keeping it the way it is.

@aymuos15, feel welcome to leave this open for a few days in case other devs feel different. :)

@stefanv
Copy link
Member

stefanv commented Oct 20, 2023

In the scipy lecture notes, we use a long integer seed, and explain somewhere why we set a seed in examples.

@aymuos15
Copy link
Author

Thank you @stefanv @lagru for the explanation and information. I will keep it open the for the time being.

@mkcor
Copy link
Member

mkcor commented Nov 13, 2023

I'm in favour of

  • either closing this PR,
  • or editing it so we follow what @stefanv wrote about the scipy lecture notes:

we use a long integer seed, and explain somewhere why we set a seed in examples.

@lagru
Copy link
Member

lagru commented Nov 18, 2023

@aymuos15, do you want to update the PR or make a new one according @mkcor's suggestion? :)

@aymuos15
Copy link
Author

Hi all. Apologies for the late response.

I will be in favour of: "we use a long integer seed, and explain somewhere why we set a seed in examples."

Will edit this PR by the end of next week, I am away at the moment. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants