-
Notifications
You must be signed in to change notification settings - Fork 393
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 Hybrid Unitary Operating Settings Initialization #8422
Conversation
…gyPlus into hybridunitary-rtf
@matthew-larson @lgentile it has been 28 days since this pull request was last updated. |
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 looks good from a code standpoint and thanks for the unit test. The one nitpick I would say is that if you are moving that loop around, it might've been good to go ahead and make it a range based loop. That change reduces the possibility of bugs from indexing. I'll pull develop in and test it out with the defect file and this should be able to go in.
I did not see a defect file on the issue or in the dev support repo, so I didn't run that. But I did pull develop in and ran unit tests, including the new one here. I am going to merge this. Thanks @matthew-larson |
Thanks @Myoldmopar for the feedback, that makes sense. |
Pull request overview
SetOperatingSettings
function to before the if statement that determines whether to go into full standby mode or step into theSetOperatingSettings
function. The before and after screenshots of the runtime fractions for the 2 operating settings during this condition are shown below.Before:
After:
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.