Gaussian Troubleshoot Combinations Improvement #670

calvinp0 · 2023-06-06T05:33:43Z

The current version of ARC cannot fully troubleshoot Gaussian due to lack of combination troubleshooting. This PR attempts to rectify this issue.

For example, if a job receives a first error of L401/301, and so we remove the chkfile by changing guess=read to guess=mix but then it its next failed run it gives another error where we need to add scf=(nosymm), then this modification will create an input.gjf file that uses BOTH troubleshooting methods.

codecov · 2023-06-06T06:27:09Z

Codecov Report

Attention: 71 lines in your changes are missing coverage. Please review.

Comparison is base (7f5190d) 73.54% compared to head (d8420a7) 73.67%.

❗ Current head d8420a7 differs from pull request most recent head 66dc921. Consider uploading reports for the commit 66dc921 to get more accurate results

Files	Patch %	Lines
arc/job/trsh.py	72.22%	10 Missing and 10 partials ⚠️
arc/job/adapters/gaussian.py	51.28%	17 Missing and 2 partials ⚠️
arc/job/adapters/common.py	70.58%	7 Missing and 8 partials ⚠️
arc/scheduler.py	0.00%	9 Missing ⚠️
arc/job/adapters/gaussian_test.py	93.33%	0 Missing and 5 partials ⚠️
arc/species/species.py	0.00%	2 Missing ⚠️
arc/imports.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #670      +/-   ##
==========================================
+ Coverage   73.54%   73.67%   +0.13%     
==========================================
  Files          99       99              
  Lines       26620    26866     +246     
  Branches     5545     5593      +48     
==========================================
+ Hits        19578    19794     +216     
- Misses       5672     5679       +7     
- Partials     1370     1393      +23

Flag	Coverage Δ
unittests	`73.67% <76.94%> (+0.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kfir4444

Thanks @calvinp0!
a quick comment, other than that it looks great!

arc/species/species.py

alongd

Thanks @calvinp0 for taking the time to fix and improve out troubleshooting!
I made some suggestions in the comments.

@Lilachn91 says that we have an issue with the nosym keyword in Gaussian and that is is misplaced when creating input files. Perhaps she can contribute a commit to this PR to fix that?

alongd · 2023-06-07T01:34:15Z

arc/job/trsh.py

        elif 'SCF' in job_status['keywords'] and 'scf=NoDIIS' not in ess_trsh_methods:
            # Switching off Pulay's Direct Inversion
            logger.info(f'Troubleshooting {job_type} job in {software} for {label} using scf=NoDIIS')
            ess_trsh_methods.append('scf=NoDIIS')
            trsh_keyword = 'scf=NoDIIS'
+            if 'CheckFile' in job_status['keywords'] or 'checkfile=None' in ess_trsh_methods:
+                remove_checkfile = True


I wonder if we can (A) put these lines somewhere where they'll be always called but just written once, and (B) generalize them for any previous trsh keyword and not just checkfile

Another thought: what about creating a small class of Trsh that stores the used trsh keywords and specifies which keyword should be kept until explicitly removed, and which should only be tried/used once. This class could be an attribute of a Species, and passed around to the Job. It'll will create a "memory" of trsh methods used. trsh here should manipulate this object (add/remove keywords). I think that ultimately this is more sustainable and the implementation is keyword/ess independent.

Yes, I think a class of Trsh would be more manageable long term, and much easier to handle when it comes to writing input files for the ESS. However, I think it would be a bit more work as it would be impacting a couple of parts of ARC. So maybe for another PR we could talk about doing that?

Would you like to put all of these (basically identical) likes in one place?

Sorry, I don't understand. Do you mean the SCF stuff into one place?

I meant that the line if 'CheckFile' in job_status['keywords'] or 'checkfile=None' in ess_trsh_methods: repeats itself many time throughout this block. We should probably have a small function that does this (with a test?), and call that function wherever needed.

I have attempted to turn all these if statements (when possible) into functions

ARC/arc/job/trsh.py

Line 1540 in 66b5d1e

def trsh_keyword_checkfile(job_status, ess_trsh_methods, couldnt_trsh) -> bool:

arc/species/species.py

arc/job/adapters/gaussian.py

alongd · 2023-06-07T01:50:49Z

arc/job/adapters/gaussian.py

@@ -230,7 +230,13 @@ def write_input_file(self) -> None:
        input_dict['method'] = self.level.method
        input_dict['multiplicity'] = self.multiplicity
        input_dict['scan_trsh'] = self.args['keyword']['scan_trsh'] if 'scan_trsh' in self.args['keyword'] else ''
+        input_dict['trsh'] = self.args['trsh']['trsh'] if 'trsh' in self.args['trsh'] else ''


We have many instances where we try to retrieve a value from self.args, but we must first check that it contains the keys.
At some point we should implement a simple function that gets key_1 and key_2 and does all the checks for args in one place, and returns the requested value if it exists. It'll make the rest of the code more elegant. You don't have to implement it as part of this PR, just a thought

arc/job/adapters/gaussian_test.py

arc/job/trsh_test.py

alongd · 2023-06-07T01:55:32Z

arc/job/trsh.py

@@ -851,66 +851,110 @@ def trsh_ess_job(label: str,
        couldnt_trsh = True

    elif software == 'gaussian':
+        trsh_keyword = [] # TODO: Current making this a list to allow for multiple keywords to be tried. Will need another approach


maybe the Trsh class approach could assist here?
BTW, I prefer not to commit TODO comments (sorry for the hypocrisy, I know I have done so myself more than a few times)

Yeah, i think this is a good example of trsh class

I can remove the TODO - it was more a note to you and kfir if you guys had another idea for this current PR

arc/job/adapters/gaussian_test.py

alongd

Thanks, I added some comments

arc/job/adapters/gaussian.py

alongd · 2023-06-08T06:34:49Z

arc/job/trsh.py

        elif 'SCF' in job_status['keywords'] and 'scf=NoDIIS' not in ess_trsh_methods:
            # Switching off Pulay's Direct Inversion
            logger.info(f'Troubleshooting {job_type} job in {software} for {label} using scf=NoDIIS')
            ess_trsh_methods.append('scf=NoDIIS')
            trsh_keyword = 'scf=NoDIIS'
+            if 'CheckFile' in job_status['keywords'] or 'checkfile=None' in ess_trsh_methods:
+                remove_checkfile = True


Would you like to put all of these (basically identical) likes in one place?

arc/job/adapters/common.py

arc/job/trsh.py

alongd

Thanks for this addition! Please see some comments below

alongd · 2023-11-14T14:53:10Z

arc/job/adapter_test.py

@@ -364,7 +364,7 @@ def test_add_to_args(self):
                         'trsh': {}}
        self.assertEqual(self.job_1.args, expected_args)
        self.job_1.write_input_file()
-        new_expected_args = {'keyword': {'general': 'val_tst_1 val_tst_2     val_tst_3 scf=xqc'},  #  scf=xqc
+        new_expected_args = {'keyword': {'general': 'val_tst_1 val_tst_2     val_tst_3'},  #  scf=xqc


Can you also remove the comment?

arc/job/adapters/common.py

alongd · 2023-11-15T04:19:50Z

arc/job/adapters/common.py

+                        if key not in input_dict.keys():
+                            input_dict[key] = ''
+                        input_dict[key] += f'{val} '
+                elif key == 'trsh':


Thanks, this is an important addition. Could you find some place in a docstring where it makes sense, and document the desired structure of args, and secifically the bahavior of trsh in it to help maintain it?
See other instances for example, here and here.

I don't know the full expectation of the desired structure of arguments. This was just to rectifiy an issue that I came across month ago.

arc/job/adapters/common.py

alongd · 2023-11-15T04:31:44Z

arc/job/adapters/gaussian.py

+        if parameters:
+            input_dict['trsh'] += f" scf=({','.join(parameters)})"
+
+        input_dict =  { key.strip(): (val.rstrip() if isinstance(val, str) and val.startswith('\n') and not val.endswith('\n') else


this part:

(val.rstrip() if isinstance(val, str) and val.startswith('\n') and not val.endswith('\n') else val.lstrip() if isinstance(val, str) and not val.startswith('\n') and val.endswith('\n') else val.strip() if isinstance(val, str) and not val.startswith('\n') and not val.endswith('\n') else val)

has some level of complexity. I recommend to extract it into a function in job.common with some tests, and use ti here inside the loop/generator

arc/job/adapters/gaussian_test.py

alongd · 2023-11-15T04:34:32Z

arc/job/trsh.py

@@ -849,60 +849,61 @@ def trsh_ess_job(label: str,
                 or 'is not appropriate for the this chemistry' in job_status['error']):
        output_errors.append(f'Error: Could not recognize basis set {job_status["error"].split()[-1]} in {software}; ')
        couldnt_trsh = True
-
+


remove these empty lines above?

alongd · 2023-11-15T04:37:01Z

arc/job/trsh.py

@@ -1536,3 +1537,72 @@ def scan_quality_check(label: str,
            return invalidate, invalidation_reason, message, actions

    return invalidate, invalidation_reason, message, actions
+
+def trsh_keyword_checkfile(job_status, ess_trsh_methods, couldnt_trsh) -> bool:


The return types here and below are tuples of bool and other things, something like: Tuple[bool, list, bool]

arc/job/trsh.py

alongd · 2023-11-15T11:49:31Z

arc/job/adapters/common.py

@@ -380,3 +451,39 @@ def which(command: Union[str, list],
        return bool(ans)
    else:
        return ans
+
+def combine_parameters(input_dict: dict, terms: list):


Minor: Please add the returned type: -> Tuple[dict, list]

pytest will always be in sys.modules even if debug is not run.

${trsh} has been added to the input template Added the integral algorithm change - this was not done previously Will change qc to xqc if it exists in the input_dict['trsh'] - this change will mean fundamental changes to the original input whereby not using xqc/qc to begin with. if we want this defaulted, we should just make it default changed the fine logic - if set to fine, then it will look for current scf in the input_dict['trsh'] and edit it

trsh tests have been updated

Large updates including determining the trsh/keywords/block values Also created a combine parameters function Adjusted input_dict_strip and added hints

alongd

Thanks!

github-actions bot added Module: Scheduler Module: Species Module: trsh Troubleshooting labels Jun 6, 2023

calvinp0 requested review from alongd and kfir4444 June 6, 2023 05:34

calvinp0 changed the title ~~Trsh scan 301~~ Gaussian Troubleshoot Combinations Improvement Jun 6, 2023

kfir4444 reviewed Jun 6, 2023

View reviewed changes

arc/species/species.py Show resolved Hide resolved

arc/species/species.py Show resolved Hide resolved

alongd reviewed Jun 7, 2023

View reviewed changes

calvinp0 force-pushed the trsh_scan_301 branch from 33ebef2 to 8538659 Compare June 7, 2023 09:04

alongd reviewed Jun 8, 2023

View reviewed changes

github-actions bot added the Module: Mapping label Jun 12, 2023

github-advanced-security bot found potential problems Jun 12, 2023

View reviewed changes

arc/job/adapters/common.py Fixed Show fixed Hide fixed

calvinp0 force-pushed the trsh_scan_301 branch from 36273f4 to bdc66a6 Compare June 24, 2023 12:46

calvinp0 force-pushed the trsh_scan_301 branch from bdc66a6 to b3a7e97 Compare July 4, 2023 10:40

calvinp0 force-pushed the trsh_scan_301 branch from e592590 to 37ad8b9 Compare November 12, 2023 23:17

github-advanced-security bot found potential problems Nov 14, 2023

View reviewed changes

arc/job/trsh.py Fixed Show fixed Hide fixed

arc/job/trsh.py Fixed Show fixed Hide fixed

calvinp0 requested review from alongd, kfir4444 and Lilachn91 November 14, 2023 08:05

calvinp0 force-pushed the trsh_scan_301 branch 2 times, most recently from ac34aef to 6b1be3a Compare November 14, 2023 10:00

alongd reviewed Nov 15, 2023

View reviewed changes

calvinp0 added 6 commits November 18, 2023 12:20

Added trunk extension to ignore list

9dac072

Removed 'pytest' search in imports

c5fa2d3

pytest will always be in sys.modules even if debug is not run.

Removed comment of scf=xqc

9c43def

Gaussian Test: Added new trsh tests

8f55e97

Created trsh functions for gaussian

52fbf20

calvinp0 added 5 commits November 18, 2023 12:34

trsh test

4073d68

trsh tests have been updated

Better troubleshooting in the scheduler

4795359

Fixed number of heavy atoms helper function

d50770e

Added l301 and l401 output files for tests

1bda430

Adjusted orca tests due to common.py changes

dfa87df

calvinp0 force-pushed the trsh_scan_301 branch from 501011a to 8dab219 Compare November 18, 2023 10:37

calvinp0 requested a review from alongd November 18, 2023 13:08

calvinp0 added 3 commits November 18, 2023 17:19

Common and Common Tests

c4fe208

Large updates including determining the trsh/keywords/block values Also created a combine parameters function Adjusted input_dict_strip and added hints

Fix formatting

62b1a37

Added to gitignore

66dc921

calvinp0 force-pushed the trsh_scan_301 branch from d8420a7 to 66dc921 Compare November 18, 2023 15:19

alongd approved these changes Nov 19, 2023

View reviewed changes

alongd merged commit 26b200b into main Nov 19, 2023

alongd deleted the trsh_scan_301 branch November 19, 2023 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gaussian Troubleshoot Combinations Improvement #670

Gaussian Troubleshoot Combinations Improvement #670

calvinp0 commented Jun 6, 2023 •

edited

Loading

codecov bot commented Jun 6, 2023 •

edited

Loading

kfir4444 left a comment

alongd left a comment

alongd Jun 7, 2023

calvinp0 Jun 7, 2023

alongd Jun 8, 2023

calvinp0 Jun 8, 2023

alongd Jun 11, 2023

calvinp0 Nov 14, 2023

alongd Jun 7, 2023

alongd Jun 7, 2023

calvinp0 Jun 7, 2023

alongd left a comment

alongd Jun 8, 2023

alongd left a comment

alongd Nov 14, 2023

alongd Nov 15, 2023

calvinp0 Nov 15, 2023

alongd Nov 15, 2023

alongd Nov 15, 2023

alongd Nov 15, 2023

alongd Nov 15, 2023

alongd left a comment

Gaussian Troubleshoot Combinations Improvement #670

Gaussian Troubleshoot Combinations Improvement #670

Conversation

calvinp0 commented Jun 6, 2023 • edited Loading

codecov bot commented Jun 6, 2023 • edited Loading

Codecov Report

kfir4444 left a comment

Choose a reason for hiding this comment

alongd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd left a comment

Choose a reason for hiding this comment

calvinp0 commented Jun 6, 2023 •

edited

Loading

codecov bot commented Jun 6, 2023 •

edited

Loading