-
Couldn't load subscription status.
- Fork 791
Recreation #756
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
Recreation #756
Conversation
Codecov Report
@@ Coverage Diff @@
## master #756 +/- ##
========================================
- Coverage 81% 81% -1%
- Complexity 2681 2713 +32
========================================
Files 100 101 +1
Lines 16802 16853 +51
Branches 2317 2324 +7
========================================
+ Hits 13699 13738 +39
- Misses 2404 2426 +22
+ Partials 699 689 -10
Continue to review full report at Codecov.
|
| clone.name = this.name; | ||
| clone.submodule = this.submodule; | ||
| clone.remarks = this.remarks; | ||
| clone.states = new ConcurrentHashMap<String, State>(); |
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 guess you could move this inside the if block.
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.
Good catch.
|
|
||
| /** | ||
| * Retuns a random double. | ||
| * Returns a random double. |
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.
Rather than having the random number related methods as public Person methods, I think it would be cleaner if they were all gathered together into an interface (e.g. RandomProvider) that Person implements. Methods that need access to randomness but not other aspects of Person could then just accept a RandomProvider instead of Person and this would make it clear which methods need Person access vs just Randomness.
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.
An interface like RandomProvider is a good idea. I'll work on that.
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 called the interface RandomNumberGenerator because I feel like Provider is overloaded with meaning, e.g. healthcare provider.
| Clinician clinician = null; | ||
| try { | ||
| Demographics city = location.randomCity(clinicianRand); | ||
| Person doc = new Person(clinicianIdentifier); |
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.
Should this be Person(clinicianSeed) instead?
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.
No. I admit that it is a little hard to follow, but clinicianSeed is the seed for ALL of the clinicians that can be passed in with the GeneratorOptions. The clinicianIdentifier is the seed for THIS particular clinician.
I'll make some changes in this area when I refactor with a RandomProvider as suggested above.
|
@hadleynet Ready for your re-review. |
|
Rebased and merged. |
This pull request attempts to address #752.
Recreation of data requires the use of a random seed (
-s), a clinician seed (-cs), and a reference date (-r).Things that will not be identical:
Summary of Changes
RandominsidePersonto beprivateand made a few new accessor methods (e.g.randBoolean()).Generator.java), that meansRandomobjects are no longer passed around as method arguments, and there should be no use ofRandomin the code.Person#rand*methods.System.currentTimeInMillis()calls except for a few spots (e.g.Generator.javaand sometimes export paths when folder per run settings are used).-rswitch forreferenceDatewhich can be used to specify when Synthea bases all the birth dates from. In my testing I used20200320(or March 20th, 2020) but you can use any date. This defaults to now (System.currentTimeinMillis()) if no value is supplied.