-
Notifications
You must be signed in to change notification settings - Fork 82
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
Orca Harness. #178
base: master
Are you sure you want to change the base?
Orca Harness. #178
Conversation
* master: (61 commits) Config: Changes new programs from job to task config Config: JobConfig -> TaskConfig Config: Adds nodes to config entos: Black reformat of entos.py and added @using_entos mark PR comments PR comments: TYPE_CHECKING, formatting, reduce redundant code entos: Added entos_policy to the input file entos: Updated commit for qcengine_records_commit hessparse and compute: Forgotten imports entos: Begun adding support for xtb in the entos Harness entos: Removed soon to be deprecated option entos: Added entos to the test_standard_suite_hf and did some minor cleanup in the entos Harness entos: Starting to add HF command to entos Fixed typo in error name Multiple changes based on review Gamess: replace print statements with logging Entos: add hessian and xtb support Parse named properties out of NWChem output Get more of the relevant fields from output Removed unncessary "pass" statement ...
This pull request introduces 3 alerts when merging 2657c79 into f9fea80 - view on LGTM.com new alerts:
|
This pull request introduces 5 alerts when merging 97c63cc into f9fea80 - view on LGTM.com new alerts:
|
yeah I know, good bot |
- Total energy is returned when using DFT. - `commands` now contains the full PATH to the orca binary. That is important to run the binary in paralell mode. - Removal/commenting of unused stuff.
This pull request introduces 2 alerts when merging e6c00c2 into f9fea80 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging ca4f623 into f9fea80 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging bd28893 into f9fea80 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging d257a35 into 16194de - view on LGTM.com new alerts:
|
* upstream/master: Qchem: SCF-> Total energy in the final basis set for correctly catching -D, etc. Qchem: PR comments Testing: Finishes up using conversion on several straglers Testing: Applies new using strategy to all files Testing: New using strategy Psi4: Fixes issue with HF3c execution PR comments PR comments: parse logfile for fields not available in QCSCR also, fix dipole parsing nwc: add hessian file; minor testing updates (MolSSI#177) PR comments: use qcel's NUMBER regex also, blacken and isort fix test until next qcel version PR comments: added ability to ingest logfile+QCSCR Blacken QCEngine PR comments: rename input, refactor NUMBER into regex snippet file Update QCENGINE_RECORDS_COMMIT Update QCENGINE_RECORDS_COMMIT Qchem: add tests for log parser for archival data Qchem: add log parser for archival data
This pull request introduces 2 alerts when merging 1e96ea0 into 9f5fba2 - view on LGTM.com new alerts:
|
- Cleaned up unneeded comments. - Added support for HF method. - Parallel computation support using %pal. - Black beautification.
This pull request introduces 2 alerts when merging 006afb8 into 9f5fba2 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 5620a66 into 9f5fba2 - view on LGTM.com new alerts:
|
* master: (21 commits) Docs: Changelog 0.13.0 extension Psi4: Ensures output data for psi4, fixes MolSSI#176 Docs: Adds v0.13.0 changelog Lint: Fixes GHA routine Testing: Canonicalizes from testing, closes MolSSI#193 Lint: Adds a GHA action to check for Black GitHub: Updates tempaltes to QCArchive standard Lint: Blackens code base Addresses MolSSI#186 Improved MPI support Clarified documentation Improved documentation and type checking for node configuration Added ability to forward output from popen Documentation for configs needed for node-parallel tasks Removed unused import Bug fix: Use integer division not floating point Read from both stderr and stdout to prevent buffer deadlock Bug fix: Memory is per MPI rank Bug fix: Missing task configuration when making mpi command Improved warnings for when multi-node jobs are misconfigured ...
This pull request introduces 2 alerts when merging 4a125bc into 30030d5 - view on LGTM.com new alerts:
|
* master: NWCHEM: parse dispersion_correction output from psivar/ fix bug Read Dispersion Energy from nwchem output file and add it to psivar Update harvester.py
This pull request introduces 2 alerts when merging 9dfc4e6 into fcd6f3d - view on LGTM.com new alerts:
|
* master: (29 commits) CI for NWChem (MolSSI#212) Make sure input extras tags get to the output Added NWChem to the canonical harness tests Calls to rtdb must use all threads Remove assumption that NWChem uses original order of atoms Fixed test for cores_per_rank Correctly determine memory sizes, when MPI should be used Account for cores_per_rank when computing total ranks Added a more difficult test for the nwchem hessian Read hessian array in proper order Use pep8 variable names Rotate gradients and hessian to match input molecule Linted with black Added Hessian support and improved gradient accuracy Allow for methods that are combinations of several functionals psi4 windows version and outfile psiapi mode (MolSSI#201) NWCHEM: added keyword dft to xc_functionals list. If method == "dft" (MolSSI#204) entos: Updated QCER hash and program_overview.rst black reformatting entos: AO reordering added to unrestricted wavefunction properties ...
- Removed Molpro references. - Removed restricted/unrestricted case guessing.
@dgasmith would it be possible to have the first review to know what steps to take forward? The only check not passing right now is the CI. I would not know how to do this for Orca because it has a very particular way to be installed :(. |
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.
Testing Orca with CI will indeed be tricky since one needs a license. But before that problem, it'd be good to add some tests locally so you know this is running. You can add it to here (https://github.com/MolSSI/QCEngine/blob/fd086f628bf9e18e1b6ea1e8e78a6623f209b934/qcengine/tests/test_harness_canonical.py) which smoke tests operation. There's also the standard_suite tests, which see if answers match other programs. And whatever other independent tests you like, as some other programs have. To address the CI situation, we'll probably have you add to the QCEngingRecords repo, which tests input writing and parsing. But since you have Orca locally, tests in this repo are the place to start.
qcengine/programs/orca.py
Outdated
# caseless_keywords = {k.lower(): v for k, v in input_model.keywords.items()} | ||
|
||
# Write the geom | ||
xyz_block = input_model.molecule.to_string(dtype="orca", units="Angstrom") |
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.
there isn't an orca dtype at the moment. you can add one here or work with me so I can write one.
also, running in bohr is strongly preferred since atomic units don't change with codata revisions.
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.
Along with this PR there is another PR for QCElemental. I will create it soon. I will change to bohr instead. Let me research about it.
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.
Overall, this is looking really good!
- `conversion_factor()` function used to convert from eV to Hartree. - cclib is just imported when invoking `.parse_output()`. - `.get_gradient()` renamed to `._get_gradient()` so that it becomes a private function.
This pull request introduces 1 alert when merging 21ae056 into e370e86 - view on LGTM.com new alerts:
|
* origin/master: simplify better error message from get_program, closes MolSSI#213 pass for psi w/o --module prep env for psithon or psiapi, get the other for free psithon _or_ psiapi for p4
This pull request introduces 1 alert when merging adba913 into d31f65b - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging e192143 into d31f65b - view on LGTM.com new alerts:
|
* master: (40 commits) Docs: Minor changelog tweaks based on comments remove the unnecessary if else blocks Docs: Cleans up code blocks OpenMM: Switches method/basis as discussed Docs: Adds notes on molecular mechancs programs Docs: Patches up doc errors Docs: Adds OpenMM to the overview Docs: Adds 0.14.0 changelog fix logic only check Nalpha ==Nbeta if already in psivar OpenMM: Now requires openforcefield as well make psi4 harness safe for psiapi file cleaning CI: Re-enables TorchANI CI testing CI: Removes duplicate RDKit build OpenMM: Adds tests for other openff, ffs Psi4: Bumps dev branch to 500 Testing: Patches up tests after QCElemental sparse molecule changes OpenMM: Fixes forces and adds an optimization example OpenMM: Adds openmm tests to canonical tests OpenMM: Simplifies method/basis in accordance with MolSSI#192 OpenMM: Testing now depends on rdkit ...
This pull request introduces 2 alerts when merging d505c4b into c8ae155 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 2b69b56 into c8ae155 - view on LGTM.com new alerts:
|
* master: fixed format add procedure for pyberny geometry optimizer Add MDI version check Linted mdi_server.py Update to support MDI v1.1.3 added optional param for execute: exit_code add conda-envs readme fixing my h20v2 mistake add homo lumo test added homo lumo test caputure +- 0 and exponent all at once, remove if-else format comment for clarity if else for for +/- exponent
This pull request introduces 2 alerts when merging 2958358 into 4267201 - view on LGTM.com new alerts:
|
Hi, what is the status of this PR. Could we merge it? |
Just for your information, I manually added this PR into my local fork. And orca is working correctly on my end. |
Hi, I'm not very familiar with the QCArchive codebase and found difficulties when run orca with qcarchive. The problem is that I managed a workaround by adjusting the configuration as follows. From Parsl's perspective, this ensures: # manager.yaml
common:
adapter: parsl
tasks_per_worker: 1
cores_per_worker: 1 # this is a workaround for parsl because it determine cpus_per_task = cores_per_node / tasks_per_node, we need cpus_per_task=1
memory_per_worker: 192
max_workers: 20
scratch_directory: "$SCRATCH"
cluster:
node_exclusivity: True
scheduler: slurm
scheduler_options:
- --ntasks-per-node=56 # this is because orca uses multi-processing instead of multi-threading however, I would now need to mannually hack the orca.py to set the number of process # parallel = "%pal nproc {} end".format(config.ncores)
config_ncores = 56
parallel = "%pal nproc {} end".format(config_ncores) because the launch command Any guidance or improvements to fix this issue would be appreciated! |
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 for pursuing this. Please go ahead and rebase so that CI can run.
_output = output["stdout"].split() | ||
|
||
for index, line in enumerate(_output): | ||
if "Version" in line: | ||
found = True | ||
if found: | ||
version = _output[index + 1] | ||
break |
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.
Shouldn't this block be indented so that it only runs after success, output = execute(
?
|
||
# Write appropriate driver call | ||
if input_model.driver == "energy": | ||
input_file.extend(energy_call) |
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.
Is energy_call
var ever non-empty?
input_file.append("! {} {}".format(input_model.model.method, input_model.model.basis)) | ||
input_file.append(xyz_block) | ||
|
||
elif input_model.model.method.upper() in self._dft_functionals or self._hf_methods: # DFT case |
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 don't think this is going to catch self._hf_methods
>>> aa = [1, 2, 3, 4]
>>> bb = [5, 6, 7, 8]
>>> 2 in aa or bb
True
>>> 7 in aa or bb
[5, 6, 7, 8]
|
||
def parse_output(self, outfiles: Dict[str, str], input_model: "AtomicInput") -> "AtomicResult": | ||
try: | ||
import cclib |
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.
It's preferred to detect needed deps in the found
function so that compute has the best chance to run cleanly. There's an example with networkx in nwchem/runner.py .
Description
Orca Harness. It goes along with MolSSI/QCElemental#186.
Status