-
Notifications
You must be signed in to change notification settings - Fork 23
Gauss Trsh Fixes #713
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
Gauss Trsh Fixes #713
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #713 +/- ##
==========================================
+ Coverage 73.66% 73.68% +0.01%
==========================================
Files 99 99
Lines 26866 26903 +37
Branches 5593 5604 +11
==========================================
+ Hits 19792 19824 +32
- Misses 5680 5685 +5
Partials 1394 1394
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
1fb01a1
to
f108f62
Compare
Opt is now treated like SCF, whereby param terms are split and recombined into one `opt=(...)` section Added NoSymm
NoSymm is not meant to be part of the opt or scf paramter combinations
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.
Looks good, thanks!
I'm missing a test generating a Gaussian input file with the nosymm keyword properly placed
@@ -522,4 +522,15 @@ def combine_parameters(input_dict: dict, terms: list) -> Tuple[dict, List]: | |||
value = re.sub(term, '', value) | |||
input_dict_copy[key] = value | |||
|
|||
# Parameters may appear as one word, so need to split them via comma |
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.
Nice addition. Can you add some tests?
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.
In the previous PR that was merged, we implemented tests for the function
ARC/arc/job/adapters/gaussian_test.py Line 525 in e407531
@alongd |
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 fix!
@Lilachn91 pointed out that
nosymm
should not be part of the opt or scf combination of parameters but be its own parameter. This has been implemented. Furthermore, at times we end up creating an input file that may haveopt=([param1])
andopt=([param2])
when rather they should be combined into oneopt=([param1,param2])