-
Notifications
You must be signed in to change notification settings - Fork 23
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
Updated args['trsh'] if condition #721
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #721 +/- ##
==========================================
- Coverage 73.72% 73.71% -0.02%
==========================================
Files 99 99
Lines 26985 26992 +7
Branches 5616 5620 +4
==========================================
+ Hits 19896 19898 +2
- Misses 5691 5697 +6
+ Partials 1398 1397 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thanks for the quick response for this issue.
LGTM. I added some comments, mostly on style.
arc/scheduler.py
Outdated
args['trsh'][key].extend(value) | ||
else: | ||
args['trsh'][key] = value | ||
else: |
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.
Both else
lead to the same action, please consider combining the if
statements and keep just one else
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.
As far as I can tell, they wouldn't. Because if ['trsh] in key
does exist, but it is not a list, (and assuming we have removed the else statement) then nothing will happen, which is not what we want.
arc/scheduler.py
Outdated
else: | ||
args['trsh'][key] = value | ||
else: | ||
args['trsh'] = trsh |
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 we think this is OK, since trsh
is a dict?
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.
Yes, it becomes a dict within a dict
For example
args['trsh]['trsh']
or
args['trsh]['scan_res']
44f498f
to
dccf3ca
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.
Thanks for addressing this quickly!
When a scan job was being troubleshooted, it was not removing the scan resolution troubleshoot correctly prior to writing the input gaussian file. This change in the code ensures that it will be removed correctly later in the code.