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

[stdlib] rename variance parameter to std (standard deviation) #4118

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

illiasheshyn
Copy link

addresses the following issue regarding the argument name in randn_float64() as well as randn.

The choice to go with standard deviation over variance was made based on the existing random.gauss function from Python's standard library and numpy's random.normal which both take mean and standard deviation.

Things to consider

  • the new argument name could arguably be more descriptive.
  • the added test does increase the testing time by 2-3 seconds on my machine.

Changes

  • renames the argument for randn_float64() and randn
  • extends the corresponding unit tests

@soraros
Copy link
Contributor

soraros commented Mar 6, 2025

Can we move this test to a separate function (for this is a borderline quality test) and use seed to reduce the number of iterations (and thus the run time)?

…y distributed random values

Signed-off-by: Illia Sheshyn <[email protected]>
Signed-off-by: Illia Sheshyn <[email protected]>
@illiasheshyn illiasheshyn force-pushed the update-random-normal-arg-variance-name branch from b76ccc1 to c3c6e5c Compare March 7, 2025 03:05
@illiasheshyn
Copy link
Author

@soraros thanks for the quick review! Do the changes align with what you had in mind?

return external_call["KGEN_CompilerRT_NormalDouble", Float64](
mean, variance
)
return external_call["KGEN_CompilerRT_NormalDouble", Float64](mean, std)
Copy link
Contributor

Choose a reason for hiding this comment

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

Uhm, can you really just change that? if the underlying function expects a variance and you give it a standard deviation... you'll get another distribution than what you're expecting. You'd have to square the value for this to make sense.

Copy link
Contributor

@soraros soraros Mar 7, 2025

Choose a reason for hiding this comment

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

I thought the bug was that the underlying function expects a std, and the argument was wrongly named?

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, didn't read the linked issue

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.

3 participants