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

Reproducibility fixes #1237

Merged
merged 7 commits into from
Feb 3, 2023
Merged

Reproducibility fixes #1237

merged 7 commits into from
Feb 3, 2023

Conversation

dehall
Copy link
Contributor

@dehall dehall commented Jan 11, 2023

Addresses #1236 and fixes some instances where running the same population multiple times produces different results

Issues identified and fixed so far:

  1. The ordering of allergy reactions
  2. DocumentReference content and Insurance plan selection

(Essentially both of these are the result of using Sets where order is not guaranteed. The current fix is not ideal, it does some extra sorting at the end, so there may be a more "root" fix further upstream. Note that I was not able to reproduce this second issue when running with only a single thread, which also suggests to me further analysis is required)

  1. Resource UUIDs are occasionally different
    This is because the UUIDs are generated using the same random number generator as used during generation. When the simulation is re-run with a different end date, the generation process may tick up an extra time stamp and roll a few extra random numbers. The fix here is to essentially remove all randomness from the FhirR4 exporter, done in two ways:
    A) Select UUIDs during the generation process, by assigning them to every HealthRecord.Entry at creation time
    B) For FHIR resources that don't align 1:1 with a synthea model class, use a new method ExportHelper.buildUUID to consistently create a UUID based on the Person, time, and a "key" for uniqueness. A few examples are the Provenance added to the end of each Bundle, DiagnosticRequests and DocumentReferences created for clinical notes, and ExplanationOfBenefits.

  2. DICOM UIDs are occasionally different
    This is because the Series object and the Instances it contains are being unintentionally leaked between patients. (Also doesn't occur when running with only a single thread.) I've put in a fix to ensure objects are always cloned, but I suspect there might be a more minimal fix possible. The "cleanest" would be an extra bit in ImagingStudy.clone() to match, for example, ObservationGroup.clone() where the observations in a list are individually cloned, but that's tougher in this case because we want certain instances to have a different/random number of objects in the list.

  3. Differences across OSes
    Observed on MacOS vs a Linux Docker image, likely to occur across other combinations as well. Patients with the same config were producing wildly different results. This was because in a couple places we were selecting a random index based on a HashMap, where sort order is not guaranteed. Seems like order is consistent for a given OS but still we shouldn't even bank on that. I replaced instances of HashMap with TreeMap for a minimal change that should ensure a consistent order.

Not currently implemented as part of this PR, I suggest we implement these separately:

  • FHIR 2 & 3 updates to match R4
  • CSV exporter to use the existing UUIDs
  • Update any other exporter that generates UUIDs
    (Do we want these to be part of this PR?)

@dehall
Copy link
Contributor Author

dehall commented Jan 11, 2023

For posterity, I used this little script to assist in testing

#!/bin/bash

rm -rf output/fhir output/fhir_* log_*.txt

for s in 1 2 3
do
  echo running $s

  ./run_synthea -s 1668660039331 -cs 1668660039331 -r 20230101 -p 83 Florida > log_${s}.txt

  mv output/fhir output/fhir_${s}

done

cd output

for file in `ls fhir_1`; do echo $file; diff fhir_1/$file fhir_2/$file ; done

@codecov
Copy link

codecov bot commented Jan 11, 2023

Codecov Report

Merging #1237 (15fbe6c) into master (340615e) will decrease coverage by 1%.
The diff coverage is 89%.

@@            Coverage Diff            @@
##             master   #1237    +/-   ##
=========================================
- Coverage        78%     78%    -1%     
- Complexity     3569    3590    +21     
=========================================
  Files           165     166     +1     
  Lines         23378   23592   +214     
  Branches       3154    3187    +33     
=========================================
+ Hits          18329   18494   +165     
- Misses         4055    4078    +23     
- Partials        994    1020    +26     
Impacted Files Coverage Δ
src/main/java/org/mitre/synthea/engine/State.java 86% <85%> (+1%) ⬆️
src/main/java/org/mitre/synthea/export/FhirR4.java 82% <86%> (+1%) ⬆️
src/main/java/org/mitre/synthea/engine/Module.java 81% <100%> (ø)
...in/java/org/mitre/synthea/export/ExportHelper.java 74% <100%> (+3%) ⬆️
...tre/synthea/export/FhirPractitionerExporterR4.java 87% <100%> (ø)
...a/org/mitre/synthea/export/HospitalExporterR4.java 90% <100%> (ø)
...va/org/mitre/synthea/modules/covid/C19Vaccine.java 70% <100%> (ø)
...gents/behaviors/planfinder/PlanFinderPriority.java 100% <100%> (ø)
...n/java/org/mitre/synthea/world/concepts/Claim.java 96% <100%> (+<1%) ⬆️
...org/mitre/synthea/world/concepts/HealthRecord.java 94% <100%> (+<1%) ⬆️
... and 43 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

… generation step, and use consistent uuid selection process for resources not mapped 1:1 to synthea data model classes
@dehall dehall changed the title WIP: Reproducibility fixes Reproducibility fixes Jan 20, 2023
@dehall dehall marked this pull request as ready for review January 20, 2023 18:06
@dehall
Copy link
Contributor Author

dehall commented Jan 26, 2023

Again for posterity, some more tidbits I used to help debug the native vs Docker differences:

Dockerfile

FROM gradle:jdk19

ENV PLATFORM="docker"
ENV TZ="America/New_York"
COPY . /home/gradle/source
COPY ./cacerts /opt/java/openjdk/lib/security/cacerts
WORKDIR /home/gradle/source
CMD ./reproducibility_test.sh

reproducibility_test.sh

#!/bin/bash

if [ "$PLATFORM" = "docker" ]; then
    TARGET=persist
    echo running in docker
else
    TARGET=output
    echo running natively
fi


rm -rf log_*.txt ${TARGET}/*

for s in 1 # 2 # 3 4
do
  echo running $s

  ./run_synthea -s 1668660039331 -cs 1668660039331 -r 20230101 -e 2023011${s} -p 83 Florida > log_${s}.txt

  # ./run_synthea -s 1 -cs 1 -r 20230101 -e 2023011${s} -p 1 Florida > log_${s}.txt

  mv output/fhir ${TARGET}/fhir_${s}
  mv log_${s}.txt ${TARGET}/
done

cd ${TARGET}

for file in `ls fhir_1`; do echo $file; diff fhir_1/$file fhir_2/$file ; done 

Then run both with

docker build -t syntheadockertest . && docker run --memory=8g --mount type=bind,source="$(pwd)"/persist,target=/home/gradle/source/persist syntheadockertest  ; ./reproducibility_test.sh

FHIR generated by the native run will be in ./output, FHIR generated by the docker run will be in ./persist

@sonatype-lift
Copy link
Contributor

sonatype-lift bot commented Feb 2, 2023

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.1

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/synthetichealth/synthea/1237.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/synthetichealth/synthea/1237.diff | git apply

Once you're satisfied, commit and push your changes in your project.

Footnotes

  1. You can preview the patch by opening the patch URL in the browser.

Co-authored-by: sonatype-lift[bot] <37194012+sonatype-lift[bot]@users.noreply.github.com>
@jawalonoski jawalonoski self-assigned this Feb 3, 2023
Copy link
Member

@jawalonoski jawalonoski left a comment

Choose a reason for hiding this comment

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

Code reviewed. Looks fine. I'm going to run some tests before merging it.

@jawalonoski jawalonoski merged commit f2add0b into master Feb 3, 2023
@jawalonoski jawalonoski deleted the repro_fixes branch February 3, 2023 19:53
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.

2 participants