Random number generation seeding structure refactor #1168
Conversation
|
Checking build and tests on GitHub because ACE-IT told me no. Please hold off review. |
Brennan1994
force-pushed
the
RandomNumberGenerationSeedingStructureRefactor
branch
from
d3f236d to
ab1a995
Compare
| @@ -618,14 +618,13 @@ private async Task<List<UncertainPairedData>> ComputeStageDamageFunctionsAsync(S | |||
|
|
|||
| ScenarioStageDamage scenarioStageDamage = new(impactAreaStageDamages); | |||
| int seed = 1234; | |||
| @@ -51,9 +51,8 @@ public static Task ComputeScenario(IASElement elem, Scenario scenario, Action<IA | |||
| return Task.Run(() => | |||
| { | |||
| int seed = 1234; | |||
| @@ -343,18 +346,13 @@ private PairedData IdentifyCentralStageFrequencyAtIndexLocation() | |||
| /// <param name="inventoryAndWaterTupled"></param> | |||
| /// <param name="profileProbabilities"></param> | |||
| /// <returns></returns> | |||
| private List<StudyAreaConsequencesBinned> ComputeDamageWithUncertaintyAllCoordinates(string damageCategory, IProvideRandomNumbers randomProvider, (Inventory, List<float[]>) inventoryAndWaterTupled, List<double> profileProbabilities) | |||
| private List<StudyAreaConsequencesBinned> ComputeDamageWithUncertaintyAllCoordinates(string damageCategory, (Inventory, List<float[]>) inventoryAndWaterTupled, List<double> profileProbabilities, bool computeIsDeterministic) | |||
There was a problem hiding this comment.
Want to talk about whether we still need this bool.
| @@ -637,7 +635,7 @@ public void ReportProgress(object sender, ProgressReportEventArgs e) | |||
| internal List<string> ProduceImpactAreaStructureDetails(Dictionary<int, string> impactAreaNames) | |||
| { | |||
| //this list will be the size of the number of structures + 1 where the first string is the header | |||
| List<DeterministicOccupancyType> deterministicOccupancyTypes = Inventory.SampleOccupancyTypes(new compute.MedianRandomProvider(), computeIsDeterministic: true); | |||
| List<DeterministicOccupancyType> deterministicOccupancyTypes = Inventory.SampleOccupancyTypes(iteration:1, computeIsDeterministic: true); | |||
There was a problem hiding this comment.
Looks like we previously wanted the mean here. now we're getting the iteration 1 sample. Are we still good?
There was a problem hiding this comment.
Add comments to note that iteration is ignored for compute is deterministic
There was a problem hiding this comment.
I have added public XML comments on all of the objects that can be sampled
|
There's more here than i can chew on. Lets chat about a few things in your office today, and we'll approve or identify changes after that |
…s://github.com/HydrologicEngineeringCenter/HEC-FDA into RandomNumberGenerationSeedingStructureRefactor
|
The suspected result changing modification here is moving from a sampling schema which samples a small number of samples from a large number of random number generators to a new schema of a single random number generator with many samples. The results appear better distributed after the changes in this request. The design choice between these two sampling schemes was presented in a code review with computer scientists outside the HEC-FDA team including Senior Computer Scientist Alex Kennedy, and checked against industry best practices as documented by Microsoft. Generating many values from a single generator is the proper use of Random.cs. "Both to improve performance and to avoid inadvertently creating separate random number generators that generate identical numeric sequences, we recommend that you create one Random object to generate many random numbers over time, instead of creating new Random objects to generate one random number." - Microsoft docs |
No description provided.