Closed xiki-tempula closed 2 years ago
Hey, awesome work! Is this work in progress or ready?
Just a few comments/questions:
The code is done. I'm testing it to see if it is robust.
Is running xTB (and switching to conda because of it) necessary during the tests? Could we not just read an expected xTB output?
So the aim is not only to test the reading but also the writing of input file. If the whole things runs then we are sure that the writing works as well. If you feel that this makes the CI too slow, I could tag it with slow and not run this in the CI.
I think the XYZ file reader/writer is not necessary, I have been using ASE for that so far.
Sorry, I'm not too familiar with the ASE. I wonder if it could read multi-frame XYZ file and return the comment line of each frame? Like I have a multi-frame XYZ
2
energy: 1.0
H 1 1 1
O 1 1 1
2
energy: 1.1
H 1 1 1
O 1 1 1
Both energy: 1.0 and energy: 1.1 needs to be extracted.
The code is done. I'm testing it to see if it is robust.
Ok, let me know when it is ready for merging.
So the aim is not only to test the reading but also the writing of input file. If the whole things runs then we are sure that the writing works as well. If you feel that this makes the CI too slow, I could tag it with slow and not run this in the CI.
It is 2-3 times slower, but it's not a deal-breaker for me, just not sure if it was necessary. Also xTB is not necessary to run Q-Force, so someone without xTB installed should also be able to run the tests locally, no? Maybe a slow version does makes sense in that case. Could we not instead also write a test to check the writing of the input file? So check reading&writing, and skip the xTB run?
Sorry, I'm not too familiar with the ASE. I wonder if it could read multi-frame XYZ file and return the comment line of each frame? Like I have a multi-frame XYZ
from ase.io import read
frames = read(path, index=':')
for frame in frames:
print(frame.positions)
print(frame.info.keys())
Should have all the info you need
It is 2-3 times slower, but it's not a deal-breaker for me, just not sure if it was necessary. Also xTB is not necessary to run Q-Force, so someone without xTB installed should also be able to run the tests locally, no?
I have tagged it with slow, so now pytest -v -m "not slow"
would not run it.
Maybe a slow version does makes sense in that case. Could we not instead also write a test to check the writing of the input file? So check reading&writing, and skip the xTB run?
I think this is doable. It is just that I think this is kind of the easiest way. I have changed the CI so that it will run the quick test first and finish off with the slow one. So it should not have too much influence on the regular testing.
WIth regard to ase, I'm a bit concerned with print(frame.info.keys()) would mess up the order, so it becomes Dihedral -123.456795827 energy 125
. I have raised an issue in gitlab ase and I will see if one could get the comment line in the ordered fashion.
Oh that indeed does sound inconvenient. Okay let's leave it as it is right now until ASE makes a change.
@selimsami I have tested this on 10 compunds and it seems to be working fine. If you are happy, we can merge this PR and fix issue when it raises. Thanks.
Ok, I guess I was too old for this and it seems that from py3.6, the dict keys are ordered by default now. I will change it to use ase.
you had older than py3.6? I thought that wouldn't work for qforce in general
@selimsami It is just that I was under the impression that the dict keys are not ordered and I didn't know that in newer python > 3.6 the dict keys are ordered.
@selimsami Ok, I have remove all xyz file related file and once the tests has passed, it is ready to be merged.
Given that I have no access to Gaussian, I have made a xTB plugin that only uses the xTB. I know this is not very useful as the user could only use xTB charge but would be a good templete if people want to work on top of it. Like user giving their own hessian or charge in the xTB format. The tests are added to the qforce-examples. The current xtb plugin could be named as Gaussian-xTB?
I have adjusted the CI as well so that the xTB could be run as part of the CI to ensure we get the same end-point result, which bumps the coverage to 60%.