-
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
Define conformer single point calculation after optimization #766
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #766 +/- ##
==========================================
- Coverage 74.09% 74.04% -0.06%
==========================================
Files 101 101
Lines 28005 28030 +25
Branches 5860 5870 +10
==========================================
+ Hits 20751 20754 +3
- Misses 5786 5802 +16
- Partials 1468 1474 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
47882b1
to
0577e94
Compare
0107934
to
0577e94
Compare
0577e94
to
17e1b1a
Compare
17e1b1a
to
9378423
Compare
conformer_sp_level
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.
Looking good! I added some minor comments and questions
arc/main.py
Outdated
@@ -169,6 +170,7 @@ class ARC(object): | |||
level_of_theory (str): A shortcut representing either sp//geometry levels or a composite method. | |||
composite_method (Level): Composite method. | |||
conformer_opt_level (Level): Level of theory for conformer searches. | |||
conformer_sp_level (str, dict, Level, optional): Level of theory for conformer sp jobs after opt. |
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.
please remove "optional" (arguments can be optional, not attributes)
Why are the types of conformer_opt_level
and conformer_sp_level
different?
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.
The difference was a mistake, it's fixed now.
arc/settings/settings.py
Outdated
@@ -91,6 +91,7 @@ | |||
|
|||
# List here job types to execute by default | |||
default_job_types = {'conf_opt': True, # defaults to True if not specified | |||
'conf_sp': False, # defaults to True if not specified |
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.
please fix the comment to False as well (also align)
if not sp_flag: | ||
content += f'Conformers for {label}, optimized at the {level_of_theory} level:\n\n' | ||
else: | ||
content += f'Conformers for {label}, single point calculation at the {level_of_theory} level:\n\n' |
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.
will ARC now save two files, for conf_opt and for conf_sp, or will it override one?
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 our previous discussion, we wanted to overwrite it, meaning conf_sp
will overwrite conf_opt
if it exists. But feel free to let me know if we need to change it.
arc/scheduler.py
Outdated
@@ -547,20 +547,27 @@ def schedule_jobs(self): | |||
continue | |||
job_list = self.running_jobs[label] | |||
for job_name in job_list: | |||
if 'conformer' in job_name: | |||
if 'conf_opt' in job_name or 'conf_sp' in job_name: |
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.
maybe check here if 'conf_' in job_name
?
arc/scheduler.py
Outdated
xyz=self.species_dict[label].conformers[i], | ||
level_of_theory=self.conformer_sp_level, | ||
job_type='conf_sp', | ||
conformer=i,) |
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.
you can remove the comma at the end here
arc/scheduler.py
Outdated
self.job_dict[label]['conf_opt'] = dict() | ||
if 'conf_sp' not in self.job_dict[label] and job_type == 'conf_sp': | ||
self.job_dict[label]['conf_sp'] = dict() | ||
if job_type == 'conf_opt': |
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 you think we can replace these four lines with
self.job_dict[label][job_type][conformer] = job # save job object
?
arc/scheduler.py
Outdated
content += job_name + ', ' | ||
elif job_type == 'conformers': | ||
elif job_type == 'conf_opt': |
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.
shall we also restore conf_sp
jobs here?
At this branch, we would like to define a new job_type during our conformer search, `conf_sp`, which will be run after the conformer optimization job. Thus, we want to distinguish them by giving a new name `conf_opt`.
9378423
to
955311e
Compare
281fa05
to
74e8d50
Compare
74e8d50
to
9444375
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!
This is a follow-up to PR #766 and aims to refine the existing implementation by adhering to a consistent job naming standard.
conformers
toconf_opt
.conf_sp
.conformer_sp_level
.conformers_after_optimization.txt
using the updated level of theory and energies specified inconformer_sp_level
, if it is being used.