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

Fix for NPE seen when sometimes trying to clone the next state #1387

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

eedrummer
Copy link
Collaborator

This PR addresses Pattern 1 as reported in #1380. It makes sure that this issue should not happen, but there is still a likely underlying issue in Synthea. Whether it is worth addressing is up for debate.

The NPE in the bug report happens when the person in the simulation is going through the AVRr Referral submodule in the Myocardial Infarction module. When looking at the debugger for the case in question, Module is on the state Priority_4_Next_Encounter_2 and trying to transition to the state Valve Surgery. However, Module is in the context of the CABG Referral submodule. When it tries to find the Valve Surgery state, it isn't there.

This is likely due to both the CABG and AVRr submodules being called referral. This PR renames them so that they both have unique names. There is likely a better fix that would ensure that these two submodules don't conflict, but I was unable to pinpoint where that fix would go and this is an obvious solution that is easily implemented.

Issue appears to be related to having two submodules with the same name
as a part of a module. This renames the submodules to have unique names.
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #1387 (d082b16) into master (6192ed7) will increase coverage by 0%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #1387   +/-   ##
========================================
  Coverage        77%     77%           
- Complexity     3685    3696   +11     
========================================
  Files           170     170           
  Lines         24005   24005           
  Branches       3331    3331           
========================================
+ Hits          18524   18566   +42     
+ Misses         4448    4408   -40     
+ Partials       1033    1031    -2     

see 23 files with indirect coverage changes

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

@arvindshmicrosoft
Copy link
Contributor

Thank you @eedrummer for your great help addressing the issue!

@dehall
Copy link
Contributor

dehall commented Nov 7, 2023

The fact that synthea could be picking the wrong submodule when there are multiple with the same name is a little scary... Does this happen more often and we just don't notice?

The approach feels good, should we take this opportunity to ensure all submodules have unique names? Across the heart surgery modules there appear to be a few more with the same name: [preoperative, postop, outcomes, medications, operative_status, operation]

@eedrummer
Copy link
Collaborator Author

Ensuring all submodules have unique names is certainly a way to avoid this.

I don't know how often this happens, but I suspect it is pretty infrequent. It's likely that there is some other condition that must be met to trigger this case. If I had to guess, it would be that the user needs to go back and forth between the submodules of the same name.

Also, for the case I worked on, the issue happened in a delay state, so the time rewind logic may also have to come into play.

@jawalonoski jawalonoski merged commit 0087993 into master Nov 21, 2023
6 checks passed
@jawalonoski jawalonoski deleted the fix-referral-npe branch November 21, 2023 15:05
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.

4 participants