BugFix: Make ARC compatible to recent RMG freq scale factor assignment change.#398
BugFix: Make ARC compatible to recent RMG freq scale factor assignment change.#398oscarwumit wants to merge 18 commits into
Conversation
1. converted a long list of if-else rules to methods that are easier to maintain and expand 2. clean up logging so that only critical information is printed to the user, and leave additional info to the developer during debugging.
1. do not calc freq factor if ARC cannot get a value from Arkane. This avoids ARC spawning unwanted jobs during testing. 2. Number the tests so it's easy to know where test fails.
xiaoruiDong
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I have some minor comments. Please take a look
| -> [1, 3, 5, 6] unique values in the intersection of l1 and l2, order following value's first appearance in l1 | ||
|
|
||
| - l1 = [1, 2, 3, 3, 5, 6], l2 = [6, 3, 5, 5, 1], order_by_first_list = ``True``, return_unique = ``False`` | ||
| -> [1, 3, 5, 5, 6] unique values in the intersection of l1 and l2, order following value's first appearance in l1 |
There was a problem hiding this comment.
I think returned result [1, 3, 5, 5, 6] is not correct. It should be [1, 3, 3, 5, 6] instead. Please also modify this in the unit test
| 'camb3lyp', | ||
| 'b97', | ||
| 'wb97', | ||
| 'wb97x', |
There was a problem hiding this comment.
Ideally, I think we'd better put all of these methods lists into a separate file for better maintenance, since they are data not scripts. Put them in settings.py is one option, another option is we create a new folder named data for storing data used in ARC. I am also willing to hear other suggestions.
There was a problem hiding this comment.
I agree with you that we should relocate this.
| logger.debug(f'basis_set: {self.basis_set}') | ||
| logger.debug(f'auxiliary_basis_set: {self.auxiliary_basis_set}') | ||
|
|
||
| self.software = get_ordered_intersection_of_two_lists(preferred_ess_order, esss)[0] |
There was a problem hiding this comment.
Is this a desired behavior? At this stage, we cannot find compatible software for the job type and LOT, according to
available_software = get_ordered_intersection_of_two_lists(compatible_software, esss)
self.software = get_ordered_intersection_of_two_lists(preferred_ess_order, available_software)[0] \
if available_software else None
Although we can assign a software, it won't be compatible, right? So this can cause an error somewhere later?
There was a problem hiding this comment.
The implementation here is trying to maintain the same behavior as what ARC currently does. I agree we should think carefully about what to do. Here are some thoughts:
(1) The idea to assign a software anyway is that ARC may not aware of the compatible ESS of a particular method the user is using. Although we have an internal list of ESS keywords, it's really hard to make sure that list is comprehensive. The solution is to try at least one software before ARC gave up.
(2) If we think it's the user's responsibility to update the level_ess dictionary in settings.py and they should always know the method they are running, then we can raise once we cannot determine any compatible ESS. This approach is safer, as it won't spawn unnecessary jobs that are likely to crash, but it will require more inputs from the user.
@alongd and @xiaoruiDong what do you think?
| """Test determine ess based on method.""" | ||
| method_0 = 'wb97xd' | ||
| ess_out_0 = common.determine_model_chemistry_type(method_0) | ||
| ess_expected_0 = ['gaussian'] |
There was a problem hiding this comment.
Should we use determine_ess_based_on_method instead of determine_model_chemistry_type?
There was a problem hiding this comment.
you are right. that was a mistake using autocomplete
|
Thanks for the review. Please take a look at the fix-up commits. |
|
@oscarwumit, thanks for pulling this off! I'm standardizing the level of theory in ARC to match that of Arkane, making it a class as well. This new class will definitely benefit from the work done here to improve the software deduction by the level. Please hold off from merging this PR for now. |
|
closed via #399 |
This PR fixed a critical bug that caused ARC to crash on the current master branch for almost all jobs without a given frequency scale factor.
The bug was first reported by @kspieks. After investigation, the cause was identified as related to the recent change in how RMG deduce frequency scale factors. In short, RMG now expects a level of theory object, but ARC passes it a string, which leads to AttributeError.
This PR relates closely to #399
In addition to the bug fix, this PR also enhances how ARC deduce ESS for jobs.
Thanks @kspieks for reporting the bug, @xiaoruiDong for debugging assistance, and @dranasinghe for going over the ESS keywords.