SSD-19 : Code changes for supporting valuset with ECL having filter expression. #1

amolht · 2020-06-23T12:53:22Z

No description provided.

…ito.

…eAndExportTest

…sv if terminology url is set

hankwallace

The whitespace formatting of new code (and some changes to existing code) needs to follow the established code. It appears that existing code uses 2 spaces. Please reformat any changes and new code to follow that.

hankwallace · 2020-07-09T01:54:19Z

Dockerfile

+
+RUN gradle build -x test
+
+CMD gradle -PmainClass=App run --args='-m _bob* -p 10'


What is the significance of the --args='-m _bob* portion here?

Is there a plan to incorporate some of the commands from the README.md into the Dockerfile and then update the README.md? Should this support more of the arguments currently in the run_synthea script?

@hankwallace

--args we are using to set different arguments for a patient generation.

I will accommodate run_synthea in Docker file

Then will add these Docker commands at end of README which will be for our pupose only, but will not go to actual pull request.

@hankwallace , Updated Docker file, now it's much shorter and simpler, also updated README for this.

hankwallace · 2020-07-09T02:00:18Z

src/main/java/org/mitre/synthea/export/CSVExporter.java

-        condition(personID, encounterID, condition);
+    	  /*condition to ignore codes other then retrieved from terminology url*/
+    	  if (!StringUtils.isEmpty(Config.get("generate.terminology_service_url"))) {
+    		  if (RandomCodeGenerator.selectedCodes.stream()


Looks like there's a mix of spaces and tabs here. Please follow whatever whitespace formatting that the file already had.

@hankwallace I will update whitespace formatting in my eclipse so will not have this sort of issues again.

@hankwallace, this white-space issue fixed on my eclipse, so now there won't be any issue around this.

hankwallace · 2020-07-09T02:01:39Z

src/main/java/org/mitre/synthea/helpers/RandomCodeGenerator.java

- * Generates random codes based upon ValueSet URIs, with the help of a FHIR terminology service
- * API.
+ * Generates random codes based upon ValueSet URIs, with the help of a FHIR
+ * terminology service API.


Why change the formatting of this comment?

Its resolved.

hankwallace

Please add a description so it's clear what the code changes accomplish.

hankwallace · 2020-07-09T02:16:30Z

src/main/java/org/mitre/synthea/helpers/RandomCodeGenerator.java

+	 */
+	@SuppressWarnings({ "unchecked", "static-access" })
+	public static Code getCode(String valueSetUri, long seed) {
+		expandValueSet(valueSetUri);


Should there be some parameter validation here first?

@hankwallace, now validating vaueSetUri, if it is wrong one using default code

src/main/resources/modules/_bobsfirstmodule.json

hankwallace · 2020-07-09T02:24:52Z

src/main/resources/synthea.properties

@@ -192,7 +192,7 @@ generate.payers.selection_behavior = random
 generate.payers.loss_of_care = false

 # Add a FHIR terminology service URL to enable the use of ValueSet URIs within code definitions.
-# generate.terminology_service_url = https://r4.ontoserver.csiro.au/fhir
+#generate.terminology_service_url = https://r4.ontoserver.csiro.au/fhir


Please don't make formatting-only changes in the PRs. This will just make it harder on the reviewer once we submit this to the forked repo.

@hankwallace I will make sure to follow existing formatting.

Hi @hankwallace, Yes they are not using eclipse default indentation formatting, they have updated for white-space only with size 2.

amolht · 2020-07-14T05:04:16Z

src/main/java/org/mitre/synthea/helpers/RandomCodeGenerator.java

-          "Unable to generate code from ValueSet URI: terminology service not configured");
+  @SuppressWarnings("unchecked")
+  public static Code getCode(String valueSetUri, long seed, Code code) {
+    if (urlValidator(valueSetUri)) {


@hankwallace, now we are validating uri prior to generating Code, if uri is not valid one we are returning default code.(last parameter for the function.)

hankwallace · 2020-07-14T15:28:05Z

README.md

@@ -85,6 +85,17 @@ Generate a list of concepts (used in the records) or attributes (variables on ea
 ./gradlew attributes
 ```

+## Build and Run synthetic patient generator using docker


hankwallace

Done with my review. Just one suggestion on exception text, but I'll approve it either way.

hankwallace · 2020-07-14T15:37:06Z

src/main/java/org/mitre/synthea/helpers/RandomCodeGenerator.java

+        valueSet = objectMapper.readValue(response.getBody(), new TypeReference<Map<String, Object>>() {
+        });
+      } catch (JsonProcessingException e) {
+        throw new RuntimeException("JsonProcessingException while parsing valueSet response");


Is there any consistency in the error messages that are being thrown? Usually, there's a pattern of Error <doing something> or Unable to <do something> in the errors. I scanned a few files, but didn't see one.

Consider using a more "readable" error (and use "error" instead of "exception") for this and the one on line 98. Maybe "Error parsing json while expanding ValueSet" here and "Error fetching response while expanding ValueSet" on line 98.

hankwallace · 2020-07-14T15:38:48Z

src/test/java/org/mitre/synthea/export/ValueSetCodeResolverTest.java

@@ -72,8 +59,7 @@ public void setUp() throws Exception {
    Provider.loadProviders(location, 1L);

    Payer.clear();
-    Config.set("generate.payers.insurance_companies.default_file",
-        "generic/payers/test_payers.csv");
+    Config.set("generate.payers.insurance_companies.default_file", "generic/payers/test_payers.csv");


This and many of the ones below are just whitespace changes, right?

There's only a few, so I'm ok with leaving them.

amolht added 10 commits June 18, 2020 15:23

Enable parameter on top of ECL

556e07c

Code refactoring for removing unused code

cd6130d

SSD19 - Updated testcases for new way of fetching valueset using Mock…

cc3a5e8

…ito.

SSD-19 : Updated formatting.

02ff886

SSD-19 : Extracted Baseurl in function for mocking in testcases.

0381eaa

SSD-19 : Updated test cases for ValueSetCodeResolverTest & CodeResolv…

f9705f7

…eAndExportTest

SSD-19 : Added change for synchronized calling

84ebfa5

SSD-19 : Added tests and refactor changes

97415bb

SSD-19 : Added condition to ignore saving other codes in conditions.c…

8398f51

…sv if terminology url is set

Added docker file

69d1aaf

hankwallace requested changes Jul 9, 2020

View reviewed changes

amolht and others added 3 commits July 9, 2020 20:10

SSD-19 : Updated code for review comments.

98baade

Update RandomCodeGenerator.java

618d537

Update RandomCodeGenerator.java

b427e03

amolht commented Jul 14, 2020

View reviewed changes

hankwallace reviewed Jul 14, 2020

View reviewed changes

hankwallace approved these changes Jul 14, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSD-19 : Code changes for supporting valuset with ECL having filter expression. #1

SSD-19 : Code changes for supporting valuset with ECL having filter expression. #1

amolht commented Jun 23, 2020

hankwallace left a comment

hankwallace Jul 9, 2020

hankwallace Jul 9, 2020

amolht Jul 9, 2020

amolht Jul 9, 2020

hankwallace Jul 9, 2020

amolht Jul 9, 2020

amolht Jul 9, 2020

hankwallace Jul 9, 2020

amolht Jul 9, 2020

hankwallace left a comment

hankwallace Jul 9, 2020

amolht Jul 9, 2020

hankwallace Jul 9, 2020

amolht Jul 9, 2020

amolht Jul 9, 2020

amolht Jul 14, 2020

hankwallace Jul 14, 2020

hankwallace left a comment

hankwallace Jul 14, 2020

hankwallace Jul 14, 2020

hankwallace Jul 14, 2020


		RUN gradle build -x test

		CMD gradle -PmainClass=App run --args='-m _bob* -p 10'

SSD-19 : Code changes for supporting valuset with ECL having filter expression. #1

Are you sure you want to change the base?

SSD-19 : Code changes for supporting valuset with ECL having filter expression. #1

Conversation

amolht commented Jun 23, 2020

hankwallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hankwallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hankwallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment