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

[Draft] seed: use Ellipsis to indicate keeping current seed #269

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MuellerSeb
Copy link
Member

We used np.nan to indicate to keep the current seed in the RNG, which is a bit of a hack IMHO.

Python provides a special built in constant to be used as placeholder: Ellipsis

In my opinion it is more intuitive to state seed=... to keep the seed in contrast to seed=np.nan since we can't use None which is a special value for seeds. Since Python provides a dedicated constant to be used as a placeholder, I would use it here.

@MuellerSeb MuellerSeb requested a review from LSchueler October 19, 2022 13:16
@MuellerSeb MuellerSeb modified the milestones: v1.5, v1.4.1 Oct 29, 2022
@LSchueler
Copy link
Member

Hmm, I don't have very strong feelings about this one. I don't see np.nan to be a hack, just a value, we decided on. I have actually never seen Python ellipses outside of slicing operations. Maybe I always ignored ellipses outside of slices, but are you not concerned that people could get confused when they see ... in an argument list?

@MuellerSeb MuellerSeb removed this from the v1.4.1 milestone Nov 3, 2022
@MuellerSeb
Copy link
Member Author

Maybe you are right. np.nan still makes sense, I was just happy about learning, that Ellipsis is used as a placeholder as described here for example: https://realpython.com/python-ellipsis/#in-short-use-the-ellipsis-as-a-placeholder-in-python

We can also keep the current behavior.

@LSchueler
Copy link
Member

Woah! You can even replace pass with ...?!?
:-) I mean, it is pretty fancy and if it would be more common, I think I would definitely be for dropping np.nan as the default argument.
I'll let you decide what to do with it.

Copy link
Member

@LSchueler LSchueler left a comment

Choose a reason for hiding this comment

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

It's up to you ;-)

@MuellerSeb MuellerSeb marked this pull request as draft November 3, 2022 09:19
@MuellerSeb
Copy link
Member Author

Will keep it as a draft for now. And yes, Ellipsis are fancy :-)

@MuellerSeb MuellerSeb changed the title seed: use Ellipsis to indicate keeping current seed [Draft] seed: use Ellipsis to indicate keeping current seed Nov 3, 2022
@MuellerSeb MuellerSeb force-pushed the fix_keep_seed_indicator branch from d1a4662 to bae13af Compare November 3, 2023 18:09
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.

2 participants