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

Fix random_number() #3156

Merged
merged 6 commits into from
Jan 23, 2024
Merged

Conversation

Shaikh-Ubaid
Copy link
Member

fixes #3128

@Shaikh-Ubaid
Copy link
Member Author

Related PR: lcompilers/lpython#2464

@@ -116,6 +116,7 @@ LFORTRAN_API void _lfortran_init_random_seed(unsigned seed)
LFORTRAN_API void _lfortran_init_random_clock()
{
srand((unsigned int)clock());
rand();
Copy link
Member Author

Choose a reason for hiding this comment

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

Please read the commit message for this change.

srand((unsigned int)clock());
rand();
lfortran_rand_seed = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is problematic for two reason:

  • The functions are slow, since they need to check the status of a global variable
  • They are not thread safe due to this (the random number generator might not be thread safe either, but that's a separate issue)
  • We do not want to initialize the seed on the first run. In Fortran, the initialization is done by random_init and random_seed. The random_number function itself does not initialize nor change the seed. Consequently, this design does not allow to change the seed, since the first random_number execution will override it.

Instead, let's move to this design:

  • _lfortran_dp_rand_num returns a random number using *x = rand() / (double) RAND_MAX; and assumes the seed is initialized
  • _lfortran_init_random_clock initializes the seed
  • In LPython we call _lfortran_init_random_clock automatically at the start of the program (we already have hooks for that)
  • In LFortran we can optionally do the same with a compiler option
  • The user must initialize the random number generator using random_init and random_seed.

Copy link
Member Author

Choose a reason for hiding this comment

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

In LPython we call _lfortran_init_random_clock automatically at the start of the program (we already have hooks for that)

Do we call _lfortran_init_random_clock() automatically? As per the example at lcompilers/lpython#2382, it seems we call the seed manually. Removing the random.seed(), removes the call to _lfortran_init_random_clock().

In LFortran we can optionally do the same with a compiler option

Could you share the compiler option we should use here? Do you mean setting the random number seed using a compiler option (for example --random-seed=0)? Shall we implement it in this PR or a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not want to complicate things at this early stage. So for LPython we should emulate what Python does, which is to call the random seed automatically, and random.random() then behaves accordingly.

For LFortran we should do whatever GFortran does.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is updated. Currently, we need to seed random_number() manually. We need to support automatic seeding (#3175). I think we can support it separately.

@Shaikh-Ubaid Shaikh-Ubaid force-pushed the fix_random_number branch 2 times, most recently from 4000351 to 555fc37 Compare January 21, 2024 23:33
@Shaikh-Ubaid Shaikh-Ubaid force-pushed the fix_random_number branch 2 times, most recently from 3a3ad27 to 11293cc Compare January 23, 2024 16:51
@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as ready for review January 23, 2024 17:12
@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as draft January 23, 2024 17:14
@@ -1100,7 +1104,21 @@ pure subroutine c_i64sys_clock(count, count_rate, count_max) &
call c_i64sys_clock(count, count_rate, count_max)
end subroutine

! srand ----------------------------------------------------------------

pure subroutine f_srand(seed)
Copy link
Contributor

Choose a reason for hiding this comment

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

This function cannot be pure, since srand writes to a global variable (to set the seed), so it is not side-effects-free. (In Fortran pure is equivalent to side-effects-free).

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think this is fine.

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as ready for review January 23, 2024 18:22
@Shaikh-Ubaid Shaikh-Ubaid merged commit 46a1595 into lfortran:main Jan 23, 2024
21 checks passed
@Shaikh-Ubaid Shaikh-Ubaid deleted the fix_random_number branch January 23, 2024 18:24
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.

Stdlib: random_number() does not return random numbers
2 participants