-
Notifications
You must be signed in to change notification settings - Fork 21
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
Allow user to input paths to previous jobs #393
base: master
Are you sure you want to change the base?
Conversation
@alongd let me know what you think of the implementation. Also, let me know where this should be added to the documentation. I have tested this out in my own work, and I think it is working as desired |
Codecov Report
@@ Coverage Diff @@
## master #393 +/- ##
==========================================
- Coverage 54.39% 54.33% -0.06%
==========================================
Files 34 34
Lines 11901 11914 +13
Branches 3628 3633 +5
==========================================
+ Hits 6473 6474 +1
- Misses 4476 4487 +11
- Partials 952 953 +1
Continue to review full report at Codecov.
|
@alongd hold off on merging this, there is one additional change that I would like to make |
freq_path = previous_job_paths[spcs_label]['freq'] | ||
output_dict[spcs_label]['paths']['freq'] = freq_path | ||
print(f'Frequency path {freq_path} added for species {spcs_label}') | ||
|
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.
Just curious, when shall we use different logs for geo
and freq
? I used to thought we can use results from freq
for both freq
and geo
.
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.
I think we use freq
only for both geo and freqs, but that's done at the adapter level, processor stores all our paths and passes them along.
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.
So, it seems like, we don't have to assign geo
and freq
separately here?
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.
I can just pass freq
if you want as long as there is no reason why the user we need to specify a separate geo
file (I can't think of any reason) and as long as ARC won't freak out about not having a file for geo
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.
let's accept both, but if geo
isn't given, override it with freq. Does that sound reasonable?
fixes #347
There are many times when a user might want to calculate a species at a higher sp level using a previously calculated geometry (for example, when updating the reference database). When this is the case, it does not make sense for ARC to recalculate the geometry. ARC will submit the new sp job, but cannot submit the Arkane job because it does not know the paths to the old geometry/frequency job.
The additions in this PR allow the user to pass a dictionary with species labels as keys, and
geo
andfreq
paths in a dictionary as values. ARC then adds these species paths if given to the output dictionary before processing the species.