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.

Sign up for GitHub

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

Jump to bottom

RNG, add seed in simulation init and initialize PRNG #1599

Merged
merged 14 commits into from Nov 11, 2023
Merged

RNG, add seed in simulation init and initialize PRNG #1599

merged 14 commits into from Nov 11, 2023

Conversation

nuclearkatie
Copy link
Contributor

No description provided.

gonuke
gonuke requested changes Oct 17, 2023
View reviewed changes
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Thanks @nuclearkatie - a few comments on consistency here.

I'm still not sure why the tests are failing.

I'm also not sure about your commit history... perhaps you've used a merge rather than rebase approach?

src/context.cc Outdated
Comment on lines 224 to 228
NewDatum("RNGInfo")
->AddVal("Seed", static_cast<int>(si.seed))
->AddVal("Stride", static_cast<int>(si.stride))
->Record();

Copy link
Member

Choose a reason for hiding this comment

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

I don't think these need to be added here as well as in the "Info" block above. I'm not sure which is better, but I'm guessing the one above will be good enough.

You may still need the static_cast though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't sure if you had an opinion about adding to Info vs their own data table. If you think the former is fine, I'll just do that

src/context.cc Outdated
Comment on lines 25 to 26
seed(kDefaultSeed),
stride(kDefaultStride),
Copy link
Member

Choose a reason for hiding this comment

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

These should be initialized in the order they are declared in the header file. You can either change all of these, or move the location in which they are ordered in the header file. Note: this is a best practice and is enforced with some compiler options. It may not be true of all current Cyclus practice.

src/sim_init.cc Outdated
Comment on lines 163 to 166
qr = b_->Query("RNGInfo", NULL);
si_.seed = qr.GetVal<int>("Seed");
si_.stride = qr.GetVal<int>("Stride");

Copy link
Member

Choose a reason for hiding this comment

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

Note above comment about where the data is added - it's in two places above and I advise one that is different from this one.

@nuclearkatie
Copy link
Contributor Author

nuclearkatie commented Oct 17, 2023 •
edited

Yeah I just git pulled without thinking (or --rebaseing). Happy to undo and fix to be rebase-y

@gonuke
Copy link
Member

gonuke commented Oct 18, 2023

Give it a shot... it may be messy and not worth it. Have you ever done an interactive rebase? That might be the cleanest way, otherwise, I think you'll have many meaningless conflicts to resolve.

@gonuke gonuke added the Feature label Oct 21, 2023
@nuclearkatie nuclearkatie marked this pull request as ready for review October 23, 2023 21:52
gonuke
gonuke requested changes Oct 24, 2023
View reviewed changes
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

A little change in the RNG initialization.

We probably need to add some testing of this, as well.

src/random_number_generator.h Outdated Show resolved Hide resolved
@gonuke
Copy link
Member

gonuke commented Nov 9, 2023

@bennibbelink is going to look into the best way to address the conda failures....

@bennibbelink
Copy link
Contributor

@bennibbelink is going to look into the best way to address the conda failures....

Needed to include the <cstdint> header in random_number_generator.h. This discussion does a good job of explaining some context. I'm not sure exactly why it broke using the newer version of g++, but I'm guessing that the compiler implicitly included some std headers and uint32_t was declared in the global namespace somewhere.

Regardless, including the header fixes the problem and is better practice anyway.

@nuclearkatie
Copy link
Contributor Author

Thanks @bennibbelink!

gonuke
gonuke requested changes Nov 10, 2023
View reviewed changes
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Probably can scale back the complexity of the tests.

src/agent.h Outdated Show resolved Hide resolved
tests/random_tests.cc Outdated Show resolved Hide resolved
@gonuke
Copy link
Member

gonuke commented Nov 11, 2023

This looks good now... planning to merge

gonuke
gonuke approved these changes Nov 11, 2023
View reviewed changes
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

LGTM

@gonuke gonuke merged commit a204e51 into cyclus:main Nov 11, 2023
5 checks passed
@nuclearkatie nuclearkatie mentioned this pull request Nov 23, 2023
Merged
1 task
@nuclearkatie nuclearkatie changed the title Add seed in simulation init and initialize PRNG RNG, add seed in simulation init and initialize PRNG Apr 26, 2024
@nuclearkatie nuclearkatie mentioned this pull request Apr 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gonuke gonuke gonuke approved these changes

@bennibbelink bennibbelink Awaiting requested review from bennibbelink

Assignees

@nuclearkatie nuclearkatie

Labels
Feature
Projects
None yet
Milestone
Random behavior - kernel
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nuclearkatie @gonuke @bennibbelink