-
Notifications
You must be signed in to change notification settings - Fork 670
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
BlueButton 2 RIF Exporter #770
Conversation
Codecov Report
@@ Coverage Diff @@
## master #770 +/- ##
==========================================
+ Coverage 80% 81% +1%
- Complexity 2886 3145 +259
==========================================
Files 113 116 +3
Lines 17459 21105 +3646
Branches 2474 2800 +326
==========================================
+ Hits 14077 17279 +3202
- Misses 2593 2985 +392
- Partials 789 841 +52
Continue to review full report at Codecov.
|
641b037
to
f214bba
Compare
c387111
to
a225518
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments on the Java side of things. Still need to run things locally and check the GMF.
} | ||
} | ||
|
||
private String trimToLength(String str, int maxLength) { |
There was a problem hiding this 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 equivalent to org.apache.commons.lang3.StringUtils.truncate(str, maxLength)
-- StringUtils is already in the repo via one or more existing dependencies so we don't have to add anything new
src/test/java/org/mitre/synthea/world/concepts/LossOfCareHealthRecordTest.java
Outdated
Show resolved
Hide resolved
Is there a file that is missing? I'm getting errors when I run Synthea with BFD export turned on:
|
Set |
@hadleynet - That worked. Thanks. |
…% of benes change their current plan each Jan.
…F IDs (BENE_ID, MBI etc) that increment per patient or claim. This allows a subsequent run of Synthea to pick up where the previous one finished by using the -c flag to import the new starting point.
…or UPDATES. This avoids an issue in the RifLoader related to batching where an UPDATE could precede an INSERT.
…r the final (chronologically) updates (1 per bene) and one for all interim updates between the initial insert and the final update. Also includes some refactoring and renaming with the aim of aligning with BFD type names.
…ounter skip criteria for outpatient and inpatient files to include encounters with coded reasons even if no diagnoses or procedures are recorded for that encounter.
…d to ensure fields are supported across all appropriate file types.
…es and between spreadsheet and code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments that I want to submit before I lose them. Will finish reviewing tomorrow
* @param stopTime end time of simulation | ||
* @throws IOException if something goes wrong | ||
*/ | ||
private void exportSNF(Person person, long stopTime) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generated a sample 100 patient population, and in the output I had a snf.csv file that was totally empty. Is that the intended functionality? (ie, should the file be blank instead of just a header row?) I did confirm the file gets written to correctly if there are rows in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you recall what params you used? In my testing I don't see a snf.csv file being created if there are no rows (vs creating an empty file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it again a couple times and I haven't see any instances where the file is empty at the end of generation. Maybe I jumped the gun and looked at the file in the editor before generation was complete one time.
One other thing I do notice though is that if you generate one population where a file such as snf.csv is present, then a second population where nothing gets written to the file, the original file stays there. This is ok (the manifest doesn't refer to the file, and the same thing can happen with the CSV exporter if you change the included/excluded files) but it might be nice to document somehow. (Though I'm not sure where such a note would best fit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BB RIF exporter is designed so that you can run Synthea multiple times to build up a population. We have a script that runs Synthea for each of the states and the resulting single sets of files then contain a national population.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks really good. I focused less on the new exporter itself (as I don't expect the broader userbase to make heavy use of that) and more on the surrounding changes. Of which there are a surprising number.
There is only one change which I think is necessary, which is the default setting for the few new config items. Anything else is a suggestion/recommendation not a requirement.
"distributed_transition": [ | ||
{ | ||
"distribution": 0.74, | ||
"transition": "Physical_Therapy" | ||
}, | ||
{ | ||
"distribution": 0.18, | ||
"transition": "Occupational_Therapy" | ||
}, | ||
{ | ||
"distribution": 0.08, | ||
"transition": "Speech_Language_Therapy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be too big to easily fix, but this randomized approach could lead to weird results, for example a patient could come from knee replacement but then get speech language therapy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not fixed, I think we should create an issue to revisit this module in a separate PR after merge.
This pull request features a number changes in order to support the RIF data dictionary required for the CMS BFD project.
Related pull requests: