Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2314 +/- ##
==========================================
- Coverage 89.82% 89.8% -0.02%
==========================================
Files 173 173
Lines 21711 21722 +11
Branches 2846 2850 +4
==========================================
+ Hits 19501 19508 +7
- Misses 1615 1619 +4
Partials 595 595
Continue to review full report at Codecov.
|
| return guess_atom_element(atomname) | ||
|
|
||
|
|
||
| SYMBOLS = re.compile(r'[0-9\*\+\-]') |
There was a problem hiding this comment.
can you comment what this is matching? [0-9] then unlimited characters?
| assert guessers.guess_atom_element('1H') == 'H' | ||
| assert guessers.guess_atom_element('2H') == 'H' | ||
|
|
||
| def test_guess_element_symbols(self): |
There was a problem hiding this comment.
look up how @pytest.parametrize works (grep around in our tests and find one). You should be able to write a list of [(input1, expected1), (input2, expected2), ...] which will make rattling off a lot of tests easy + expandable
richardjgowers
left a comment
richardjgowers
left a comment
There was a problem hiding this comment.
Looks cool but needs a little tweaking, see comments
| ('HB1', 'H'), | ||
| ('OC2', 'O'), | ||
| ('1he2', 'H'), | ||
| ('3hg2', 'H'), |
There was a problem hiding this comment.
I was going to ask for more elements to be added to the table, but then obviously it will make he and hg fail. I guess there's not many people doing MD of mercury....
There was a problem hiding this comment.
Thanks for the review! (Which I meant to say earlier). I agree that more elements would have been nice -- I originally had helium and calcium in there before realising the obvious ambiguity. As GROMOS force fields have hydrogens called HG in threonine and HE in phenylalanine, I thought element diversity could be left as a problem for a future era of simulation.
richardjgowers
left a comment
richardjgowers
left a comment
There was a problem hiding this comment.
Thanks @lilyminium , I think this improves things as much as we can do.
This is your first commit so add yourself to AUTHORS and add github handle to the top of CHANGELOG and we're good to go
2 participants
Fixes #2313
This pull request tries a bit harder to guess atom elements from names (compared to the old way of returning the first letter).
Changes made in this Pull Request:
PR Checklist