Psi4 adapter #519

kfir4444 · 2022-06-28T09:00:44Z

A functioning Psi4 adapter.

alongd

Looking good, thanks! Please see some comments and questions

arc/job/adapters/psi_4.py

arc/job/adapters/psi_4Test.py

arc/job/adapters/psi_4.py

lgtm-com · 2022-06-28T21:02:43Z

This pull request introduces 1 alert and fixes 2 when merging 09ac7f5 into ef0d5c6 - view on LGTM.com

new alerts:

1 for Unused import

fixed alerts:

2 for Unused import

alongd · 2022-06-29T00:58:35Z

LGTM eventually completed after 12 hrs... It complains about unused imports

lgtm-com · 2022-07-06T10:35:37Z

This pull request introduces 2 alerts and fixes 4 when merging fe3ceb0 into 2661d86 - view on LGTM.com

new alerts:

1 for Unused import
1 for Duplicate key in dict literal

fixed alerts:

4 for Unused import

lgtm-com · 2022-07-06T11:31:09Z

This pull request fixes 4 alerts when merging ab998cf into 2661d86 - view on LGTM.com

fixed alerts:

4 for Unused import

lgtm-com · 2022-07-10T17:31:48Z

This pull request fixes 4 alerts when merging 9ea351a into 542528b - view on LGTM.com

fixed alerts:

4 for Unused import

alongd

Thanks! Please see some comments below

arc/job/adapters/psi_4.py

alongd · 2022-07-11T04:30:27Z

arc/job/adapters/psi_4.py

+dft_spherical_points {dft_spherical_points}
+dft_radial_points {dft_radial_points}
+dft_basis_tolerance {dft_basis_tolerance}
+dft_pruning_scheme robust


Does this line work, or is it problematic as we saw?

arc/job/adapters/psi_4.py

arc/job/adapters/psi_4Test.py

alongd · 2022-07-11T04:32:46Z

arc/job/adapters/psi_4Test.py

+
+
+if __name__ == '__main__':
+    unittest.main(testRunner=unittest.TextTestRunner(verbosity=2))


please add an empty line at the end of the file

alongd · 2022-07-11T04:35:38Z

arc/job/adapters/psi_4.py

-        self.execution_type = execution_type or 'queue'
-        self.command = 'psi4.py'
+        self.execution_type = execution_type or 'incore'
+        self.command = ['psi4']


Should this be imported from the incore commands in submit.py?

arc/job/adapters/psi_4Test.py

alongd · 2022-07-11T04:39:22Z

arc/scheduler.py

@@ -54,6 +54,7 @@
                                   xyz_to_coords_list,
                                   xyz_to_str,
                                   )
+from arc.settings.settings import default_incore_adapters


important: we don't import directly from settings.py. Instead, we have from arc.imports import settings on line 34, and we use this settings dict as in lines 67-70 below. Please modify this import as well. Let's talk about this more

The above import comment is critical, please take a look

alongd · 2022-07-11T04:40:13Z

arc/job/adapters/psi_4.py

@@ -18,7 +18,7 @@
                                     set_job_args,
                                     which)
 from arc.job.factory import register_job_adapter
-from arc.job.local import execute_command, submit_job
+from arc.job.local import execute_command, rename_output, submit_job


please squash this commit into the Psi4 adapter commit

lgtm-com · 2022-07-31T18:22:55Z

This pull request introduces 2 alerts and fixes 4 when merging 5b04571 into 6dc1de3 - view on LGTM.com

new alerts:

1 for Unused import
1 for Unreachable code

fixed alerts:

4 for Unused import

lgtm-com · 2022-08-15T12:31:28Z

This pull request introduces 3 alerts and fixes 4 when merging 4f0e6a0 into 05e5ab6 - view on LGTM.com

new alerts:

1 for Unused import
1 for Unnecessary 'else' clause in loop
1 for Variable defined multiple times

fixed alerts:

4 for Unused import

lgtm-com · 2022-08-15T12:56:50Z

This pull request introduces 1 alert and fixes 4 when merging 2b0588c into 05e5ab6 - view on LGTM.com

new alerts:

1 for Unnecessary 'else' clause in loop

fixed alerts:

4 for Unused import

lgtm-com · 2022-08-15T13:27:31Z

This pull request introduces 1 alert and fixes 4 when merging 58380b4 into 05e5ab6 - view on LGTM.com

new alerts:

1 for Unnecessary 'else' clause in loop

fixed alerts:

4 for Unused import

lgtm-com · 2022-08-16T06:57:30Z

This pull request introduces 1 alert and fixes 4 when merging dc41b2a into 58b5698 - view on LGTM.com

new alerts:

1 for Unnecessary 'else' clause in loop

fixed alerts:

4 for Unused import

alongd

Thanks, @kfir4444 !
Please see some comments. Also, I'm not sure I saw a in the commits a deletion of the incore_adapters_dict when it got relocated to settings. Could you check?

alongd · 2022-08-19T14:02:04Z

arc/job/adapters/psi_4.py

@@ -194,7 +140,7 @@ def __init__(self,

        self.job_adapter = 'psi4'
        self.execution_type = execution_type or 'incore'
-        self.command = 'psi4.py'
+        self.command = ["psi4", "-i", "input.dat, -o", "output.dat"]


We should make this a single entry list, otherwise it might be confusing, e.g., this instance is missing " between input.dat and -o.
Try: ['psi4 -i input.dat -o output.dat']

alongd · 2022-08-19T14:04:33Z

arc/job/adapters/psi_4.py

@@ -263,7 +209,7 @@ def __init__(self,
        self.server = self.args['trsh']['server'] if 'server' in self.args['trsh'].keys() \
            else self.ess_settings[self.job_adapter][0] if isinstance(self.ess_settings[self.job_adapter], list) \
            else self.ess_settings[self.job_adapter]
-        self.label = self.species[0].label
+        self.species_label = self.species[0].label
        if len(self.species) > 1:


The __init__ should be cleaned very aggressively. It should call the _initialize_adapter function, and only add minor additions to it. Please see the implementation in the other adapters

alongd · 2022-08-19T14:05:39Z

arc/job/adapters/psi_4.py

+        elif self.job_type == 'freq':
+            func = 'frequency'
+        elif self.job_type == 'scan':
+            pass


if scans are not implemented yet, then shouldn't we raise the NotImplementedError as below?

alongd · 2022-08-19T14:07:45Z

arc/job/adapters/psi_4.py

+        else:
+            raise NotImplementedError(f'Psi4 job type {self.job_type} is not implemented')
+
+        dft_spherical_points = 302 if self.fine else 590


We may have got these three lines in reverse. It looks like dft_spherical_points should be 590 if a fine grid is desired, otherwise use 302.
Can you double check in the Psi4 manual that this is indeed the desired behavior for a fine grid, and correct these three lines accordingly?

alongd · 2022-08-19T14:08:50Z

arc/job/adapters/psi_4.py

+dft_basis_tolerance {dft_basis_tolerance}
+}}
+"""
+# dft_pruning_scheme robust


Is this a leftover? Should this be deleted or put in actual code?

arc/job/adapters/psi_4Test.py

alongd · 2022-08-19T14:20:36Z

arc/scheduler.py

@@ -54,6 +54,7 @@
                                   xyz_to_coords_list,
                                   xyz_to_str,
                                   )
+from arc.settings.settings import default_incore_adapters


The above import comment is critical, please take a look

lgtm-com · 2022-08-23T16:16:26Z

This pull request introduces 1 alert and fixes 3 when merging c053c3c into 58b5698 - view on LGTM.com

new alerts:

1 for Unnecessary 'else' clause in loop

fixed alerts:

3 for Unused import

lgtm-com · 2022-08-25T18:06:38Z

This pull request introduces 1 alert and fixes 3 when merging 07c3ce8 into d60a51d - view on LGTM.com

new alerts:

1 for Unnecessary 'else' clause in loop

fixed alerts:

3 for Unused import

Default as incore adapter.

arc/scheduler.py

            job_adapter = level.software
+            if level.software == None:


arc/job/adapters/psi_4.py

+from arc.job.adapters.common import (_initialize_adapter,
+                                     check_argument_consistency,
                                     set_job_args,
-                                     update_input_dict_with_args,
-                                     which)
+                                     which,
+                                     )


kfir4444 requested a review from alongd June 28, 2022 10:04

alongd reviewed Jun 28, 2022

View reviewed changes

kfir4444 force-pushed the psi4_dev_new branch from 09ac7f5 to fe3ceb0 Compare July 6, 2022 10:15

kfir4444 added the Module: Job label Jul 6, 2022

kfir4444 force-pushed the psi4_dev_new branch from fe3ceb0 to ab998cf Compare July 6, 2022 11:09

kfir4444 requested a review from alongd July 6, 2022 12:12

alongd reviewed Jul 11, 2022

View reviewed changes

kfir4444 force-pushed the psi4_dev_new branch from e1d12f5 to 4f0e6a0 Compare August 15, 2022 12:13

kfir4444 force-pushed the psi4_dev_new branch from 4f0e6a0 to 2b0588c Compare August 15, 2022 12:37

kfir4444 force-pushed the psi4_dev_new branch from 2b0588c to 58380b4 Compare August 15, 2022 13:07

kfir4444 force-pushed the psi4_dev_new branch from 58380b4 to dc41b2a Compare August 16, 2022 06:35

kfir4444 requested a review from alongd August 16, 2022 09:05

alongd reviewed Aug 19, 2022

View reviewed changes

kfir4444 force-pushed the psi4_dev_new branch from dc41b2a to c053c3c Compare August 23, 2022 15:50

kfir4444 force-pushed the psi4_dev_new branch from c053c3c to 07c3ce8 Compare August 25, 2022 17:44

kfir4444 added Topic: ESS Electronic Structure Software Type: Feature labels Aug 25, 2022

kfir4444 requested a review from alongd August 25, 2022 18:46

kfir4444 added 27 commits November 27, 2022 12:02

added psi4 to determine_ess

4b5559e

Test: determine_ess

4e934e6

updated psi4 adapter

a5883b0

Test: psi4 adapter

0779bed

added psi4 to many locations

33b248a

Default as incore adapter.

Added psi4 to parse_frequencies

2fbd59c

added psi4 to parse_normal_mode_displacement

accc97b

added psi4 to identify_ess

12189c2

Test: parse_frequencies

a4dda22

Test parse_normal_mode_displacement

a7fd517

moved default_incore_adapters

75bac26

added psii4 submit script

2bb3fd1

psi4 output file for testing.

0ebb383

!f thermo demo

3cb5f6c

added psi4 install script

2af5472

added psi4 to install_all

27399d6

!f submit.p

e0f2d76

!f scheduler

a453ee9

!f psi4

9367ac7

!f psi4

1c71002

added psi4 installation to makefile

4a08fff

!f submit

9702662

!f psi4 autoinstaller

3fe89cb

debugg local

1c9bcb6

added xtb to default_incore_adapters

3e5bd94

debugging thermodemo

b4fd444

!f settings

ee00e38

kfir4444 force-pushed the psi4_dev_new branch from 59752c2 to ee00e38 Compare November 29, 2022 09:13

github-advanced-security bot found potential problems Nov 29, 2022

View reviewed changes

alongd force-pushed the main branch from a56b0df to 58b5698 Compare May 10, 2023 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Psi4 adapter #519

Psi4 adapter #519

kfir4444 commented Jun 28, 2022

alongd left a comment

lgtm-com bot commented Jun 28, 2022

alongd commented Jun 29, 2022

lgtm-com bot commented Jul 6, 2022

lgtm-com bot commented Jul 6, 2022

lgtm-com bot commented Jul 10, 2022

alongd left a comment

alongd Jul 11, 2022

alongd Jul 11, 2022

alongd Jul 11, 2022

alongd Jul 11, 2022

alongd Aug 19, 2022

alongd Jul 11, 2022

lgtm-com bot commented Jul 31, 2022

lgtm-com bot commented Aug 15, 2022

lgtm-com bot commented Aug 15, 2022

lgtm-com bot commented Aug 15, 2022

lgtm-com bot commented Aug 16, 2022

alongd left a comment

alongd Aug 19, 2022

alongd Aug 19, 2022

alongd Aug 19, 2022

alongd Aug 19, 2022

alongd Aug 19, 2022

alongd Aug 19, 2022

lgtm-com bot commented Aug 23, 2022

lgtm-com bot commented Aug 25, 2022



		if __name__ == '__main__':
		unittest.main(testRunner=unittest.TextTestRunner(verbosity=2))

Psi4 adapter #519

Are you sure you want to change the base?

Psi4 adapter #519

Conversation

kfir4444 commented Jun 28, 2022

alongd left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 28, 2022

alongd commented Jun 29, 2022

lgtm-com bot commented Jul 6, 2022

lgtm-com bot commented Jul 6, 2022

lgtm-com bot commented Jul 10, 2022

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

lgtm-com bot commented Jul 31, 2022

lgtm-com bot commented Aug 15, 2022

lgtm-com bot commented Aug 15, 2022

lgtm-com bot commented Aug 15, 2022

lgtm-com bot commented Aug 16, 2022

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

lgtm-com bot commented Aug 23, 2022

lgtm-com bot commented Aug 25, 2022