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

Restructure reading of simulation input #188

Merged
merged 17 commits into from
May 30, 2024
Merged

Conversation

jcurtis2
Copy link
Collaborator

@jcurtis2 jcurtis2 commented Aug 7, 2023

Addressing #184 by moving input file reading and setting of run_part_opt options into run_part.F90.

@jcurtis2 jcurtis2 marked this pull request as ready for review August 29, 2023 15:20
src/run_part.F90 Outdated Show resolved Hide resolved
src/run_part.F90 Outdated
Comment on lines 678 to 683
! initialize RNG with random seed for UUID generation
call pmc_srand(0, pmc_mpi_rank())

if (.not. do_restart) then
call uuid4_str(run_part_opt%uuid)
end if
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether this code would be better outside of spec_file_read_run_part()? It doesn't really seem to be strictly related to reading the spec file. In particular, I don't like having RNG initialization hidden inside a spec-file-reading function because a caller might not expect to have their RNG state reset just because they read a spec file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, i can see this. This RNG is purely for the creation of a UUID (which has more to do with the starting of a simulation and not necessarily reading a main spec file). Would it make sense to move the UUID generation outside the spec file reading but before the MPI calls/new suggested broadcast call? To borrow the code suggestion from above:

if (pmc_mpi_rank() == 0) then
    call spec_file_read_run_part()
    call uuid_generate()
end if
call spec_file_broadcast_run_part()

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I think that structure is a good idea, with a call to pmc_srand() above that code block (or maybe immediately before uuid_generate()) with a comment saying that we are initializing it for UUID generation.

One thing that was a bit unclear with the previous code was that it would generate a UUID on all processes, but then overwrite them all by broadcasting from process 0. Your new structure above makes this clearer 🎉

src/run_part.F90 Outdated Show resolved Hide resolved
src/run_part.F90 Outdated Show resolved Hide resolved
src/partmc.F90 Show resolved Hide resolved
@jcurtis2 jcurtis2 linked an issue May 30, 2024 that may be closed by this pull request
@jcurtis2 jcurtis2 merged commit 8d46edb into master May 30, 2024
10 checks passed
@jcurtis2 jcurtis2 deleted the move_run_part_inputs branch May 30, 2024 01:44
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.

Restructure reading of simulation input in partmc.F90
2 participants