Added orca log file reading capability #47
Conversation
Codecov Report
@@ Coverage Diff @@
## master #47 +/- ##
==========================================
+ Coverage 92.58% 92.67% +0.08%
==========================================
Files 33 35 +2
Lines 3114 3179 +65
Branches 386 392 +6
==========================================
+ Hits 2883 2946 +63
- Misses 156 158 +2
Partials 75 75
Continue to review full report at Codecov.
|
|
I'm going to copy-paste a few of the test outputs for convenience, so you can see what it complains about: and |
|
@evohringer Can you also add an exclusion for data files to the whitespace linter? This can be done in the following place: The following should make more sense: - whitespace:
filefilter: ['- iodata/test/data/*'] |
|
The tests bumped into a few more things. See https://travis-ci.org/theochem/iodata/jobs/503215662#L912 |
|
Sorry @tovrstra to bother you with this. Is there a way I can check this before making a pull request? |
|
Good point. Yes, but it is not as easy as it should be. (I'm doing something about it.) You can run the following: pip install --upgrade git+https://github.com/theochem/cardboardlint.git@master#egg=cardboardlint
pip install --upgrade pylint codecov pycodestyle pydocstyle
python setup.py build_ext -i
cardboardlinter -r origin/masterThe last line will do the actual work, with the first two lines installing software, which you may already have. The third line does an in-place build, which is needed to keep pylint happy with the Python extension in iodata. Can you give this a try? If something breaks, please let me know. |
|
It works , great. Thanks. but it does not print out any errors: USING : pylint 1.6.4, astroid 2.2.4 USING : 2.5.0 USING : 3.0.0 |
|
That probably is probably because you used the It is in general a good idea to use the master branch of cloned repos only to follow up the upstream commits. For clarity, it is recommended to make a new "feature" branch in which you make your changes. This way, you can always easily compare with what you started from, or update the master branch with new upstream developments. This is a quick summary of that workflow, assuming you would start from scratch, just to sketch the idea: # Clone the primary repo first
git clone [email protected]:theochem/iodata.git
# Now `origin` refers to theochem/iodata
# Add your fork as the second remote (after making the fork on github.com)
cd iodata
git remote add qcmm [email protected]:qcmm/iodata.git
# Make a feature branch, in which a new feature will be developed
git checkout -b orca
# After some changes commit these:
git add ...
git commit ...
# Push the feature branch to your repo
git push qcmm feature
# A link will be printed on your terminal to make a pull request
# If needed make more commits in the feature branch and push them to qcmm/iodata.
# The pull request will be updated automatically.With this setup You can check your current remotes as follows, with the output I typically get as comments: git remote show
# origin
# tovrstra
git remote show origin
#* remote origin
# Fetch URL: [email protected]:theochem/iodata.git
# Push URL: [email protected]:theochem/iodata.git
# HEAD branch: master
# Remote branches:
# master tracked
# Local branch configured for 'git pull':
# master rebases onto remote master
# Local ref configured for 'git push':
# master pushes to master (up to date)
git remote show tovrstra
#* remote tovrstra
# Fetch URL: [email protected]:tovrstra/iodata.git
# Push URL: [email protected]:tovrstra/iodata.git
# HEAD branch: master
# Remote branches:
# master new (next fetch will store in remotes/tovrstra)
# Local refs configured for 'git push':
# master pushes to master (fast-forwardable)(I removed some branches for clarity.) git remote show origin
#* remote origin
# Fetch URL: git://github.com/QCMM/iodata.git
# Push URL: git://github.com/QCMM/iodata.git
# HEAD branch: master
# Remote branches:
# master tracked
# Local ref configured for 'git push':
# master pushes to master (local out of date)The following commands will show how to switch in a new clone of your fork. A verbose command prompt helps a lot to see on which branch you are working. (See e.g. https://gist.github.com/kevin-smets/8568070 for an OSX example.) # start somewhere outside a git repo.
git clone https://github.com/QCMM/iodata.git
# rewire the remotes
git remote rename origin qcmm
git remote add origin [email protected]:theochem/iodata.git
# rename your development branch
git branch -m master orca
# get back the master branch from theochem/iodata
git fetch origin master:master
# run the linters
./setup.py build_ext -i
cardboardlinter -r master
# You can commit fixes now.
# force-push the master to your repo
git push qcmm master:master -f
# Now you can close this PR.
# Time to push the feature branch to your fork:
git push qcmm orca
# You should get a link on your terminal to make a PR for this branch. |
bin/horton-convert
Outdated
| @@ -36,7 +36,7 @@ def parse_args(): | |||
| ' This only works if the input contains sufficient' | |||
| ' data for the output') | |||
| parser.add_argument('-V', '--version', action='version', | |||
| version="%%(prog)s (HORTON version %s)" % version.__version__) | |||
| version="%%(prog)s (HORTON version 2.0)" ) | |||
There was a problem hiding this comment.
We used this in the past so you could define version numbers using git tags. It was too easy to neglect to update a line somewhere and have inconsistent version numbers in the program.
The downside is that if you didn't use the travisCI infrastructure, the version.py file didn't get generated. It's probably better to use a try: ... except ImportError ... fallback to get around this instead of hard coding the number.
There was a problem hiding this comment.
Yeah, we should make this easier. I'm working on something (roberto), but is not quite ready yet.
There was a problem hiding this comment.
I did add a try: ... except ImporError as suggested by Matt. Hope this is fine. Thanks @matt-chan
|
Thanks Toon for the nice guide. If this is ok I would try it for the next implementation and then I try from scratch as you suggested. One thing which is bothering me is that installing IOData locally does not allow me to run the tests in the installed version because the "path" of iodata is not set. I guess this is done automatically in travis right?? Is there a way to make two steps locally before submitting: Sorry to bother with this stupid things. I hope that conversation also help others implementing new features in iodata. |
|
Sure, no problem. I don't fully understand your question regarding the path, but I'll try to give some answer. At the moment, the following steps are needed to run the tests in the source tree. The instructions are given for a fresh clone of the repo, which I just tested: git clone [email protected]:theochem/iodata.git
cd iodata
python3 tools/gitversion.py python > iodata/version.py
python3 -m pip install pytest cython --user
python3 setup.py build_ext -i
python3 -m pytest iodataI agree, this is not intuitive at all, and this should become easier, which I'm working on. Let me clarify a few things, so you can get around it until we have a better way:
P.S. I'll take the quote of my message out of your post for clarity. |
.cardboardlint.yml~
Outdated
| - pycodestyle: | ||
| config: .pycodestylerc | ||
| - pydocstyle: | ||
| - whitespace: |
There was a problem hiding this comment.
Can you remove this file? I think it is a backup copy that accidentally committed.
| try: | ||
| version="%%(prog)s (HORTON version %s)" % version.__version__ | ||
| except ImportError: | ||
| raise ImportError('No version.py file found')) |
There was a problem hiding this comment.
version.py should normally be present. (See my last post on how to add it.)
There was a problem hiding this comment.
If this is ok I will leave for people who just try it out after installation
There was a problem hiding this comment.
ok, until our build and development workflow is simplified, we can have this. It is convenient.
iodata/test/test_orca.py
Outdated
| def test_load_water_number(): | ||
| # test if IOData has atomic numbers | ||
| with path('iodata.test.data', 'water_orca.out') as fn_xyz: | ||
| mol = IOData.from_file(str(fn_xyz)) |
There was a problem hiding this comment.
str function may not be needed. I've seen it before but normally path objects get converted to strings when needed, safe for some corner cases maybe?
iodata/test/test_orca.py
Outdated
| mol : IOData | ||
| IOdata dictionary. | ||
|
|
||
| Returns |
There was a problem hiding this comment.
No need to document return if nothing is returned.
iodata/test/test_orca.py
Outdated
| """ | ||
| assert mol.numbers[0] == 8 | ||
| assert mol.numbers[1] == 1 | ||
| assert mol.numbers[2] == 1 |
There was a problem hiding this comment.
Again a very minor suggestion, mainly sharing it because you might like the trick:
# The following line does not start with assert, because this is done inside the assert_equal function.
np.testing.assert_equal(mol.numbers, [8, 1, 1])The module np.testing contains a lot of convenient tricks to write assertions for arrays. See https://docs.scipy.org/doc/numpy-1.13.0/reference/routines.testing.html
There was a problem hiding this comment.
Changed to np.testing. This is a nice feature. I will have a look.
iodata/test/test_orca.py
Outdated
| def check_water(mol): | ||
| """Checks if atomic numbers and coordinates obtained from orca out file are correct. | ||
|
|
||
| Parameters |
There was a problem hiding this comment.
Can you remove one space of indentation from this and following lines in the docstring. (travis tests fail on this)
There was a problem hiding this comment.
Can you please replace Checks with Check? First line should be in imperative mood.
|
A few small things:
|
All done. Thanks for checking. |
|
LGTM. Thanks! |
|
@evohringer I'll let you merge. (You should see a green button.) It is generally better to let the author of PR perform the merge, because he/she can decide best when the PR is final. |
|
No green button to merge. Everything is green but I can not find the button. The last part says: |
|
OK that's right, only few have write access to avoid unfortunate accidents. I'll do it. |
This adds a orca log output file reader which reads in the SCF energy and atomic numbers and coordinates.
This new function is tested in test_orca for which a new log file was created in the data directory.
I also changed the horton-convert script.
Please provide feedback since this would be a draft for other QC output file readers.
Refs: #43