modified guess_atom_element for more accurate guess #4168

pillose · 2023-06-14T13:51:45Z

Changes made in this Pull Request:

Suppose the function guess_atom_element() receives 'N0A' as an argument
With previous code,
name = 'NA' # interpreted as sodium
Which substitute '0' to ''
With new code,
name = ['N', 'A'][0] #== 'N"

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

📚 Documentation preview 📚: https://mdanalysis--4168.org.readthedocs.build/en/4168/

github-actions

Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on the developer mailing list so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.

github-actions · 2023-06-14T13:54:35Z

Linter Bot Results:

Hi @pillose! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location	Outcome
main package	⚠️ Possible failure
testsuite	⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/5879473341/job/15943597343

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

codecov · 2023-06-14T15:24:08Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (a14e98d) 93.40% compared to head (19511fc) 93.41%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4168      +/-   ##
===========================================
+ Coverage    93.40%   93.41%   +0.01%     
===========================================
  Files          169      183      +14     
  Lines        22202    23309    +1107     
  Branches      4064     4064              
===========================================
+ Hits         20738    21775    +1037     
- Misses         948     1018      +70     
  Partials       516      516

Files Changed	Coverage Δ
package/MDAnalysis/topology/guessers.py	`99.22% <100.00%> (+0.01%)`	⬆️

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hmacdope

Hi @pillose! Thanks for the contribution!

Would you be able to add a test of the new behaviour? If you need help getting started with the test suite, feel free to ping me. You will also need to add an entry to package/CHANGELOG.

Also as you are a new contributor (welcome!) please add yourself to package/AUTHORS and also introduce yourself on the developer mailing list or discord.

IAlibay · 2023-07-29T00:10:27Z

@pillose this PR has been opened for a little while, are you still interested in contributing this code?

pillose · 2023-07-29T21:50:37Z

@hmacdope I added some test and change log. Also, I added my name(Geongi Moon) to package/AUTHORS.
I apologize for missing your comment.

@IAlibay Thank you for the reminder.

hmacdope

Looks good, two little comments. Did you end up introducing yourself on mailing list or discord :) ?

hmacdope · 2023-07-30T10:47:17Z

package/CHANGELOG

@@ -19,6 +19,7 @@ The rules for this file:

 Fixes
  * Fix Boltzmann typo in `units.py` (PR #4214, Issue #4213)
+  * Fix Atom type guessing error (PR #4168, Issue #4167)


Order this newest first.

hmacdope · 2023-07-30T10:48:15Z

package/MDAnalysis/topology/guessers.py

@@ -204,7 +204,12 @@ def guess_atom_element(atomname):
    except KeyError:
        # strip symbols and numbers
        no_symbols = re.sub(SYMBOLS, '', atomname)
-        name = re.sub(NUMBERS, '', no_symbols).upper()
+        no_numbers = re.split(NUMBERS, no_symbols)


Add some comments as to why this is now in here.

IAlibay

@pillose - We are changing the license under which future contributions are released. Could you please confirm that you agree to releasing this code under the terms of the LGPLv2.1 and that your contribution also adheres to the developer certificate of origion?

RMeli

Thanks @pillose. Please see #4168 (review), thanks.

package/CHANGELOG

package/AUTHORS

package/MDAnalysis/topology/guessers.py

pillose · 2023-08-15T14:12:10Z

@IAlibay I agree to changing the license.
@hmacdope I just introduced myself on mailing list.
@RMeli Thank you for the reminder.

Co-authored-by: Rocco Meli <[email protected]>

hmacdope · 2023-08-16T05:59:47Z

package/AUTHORS

@@ -220,6 +220,7 @@ Chronological list of authors
  - Mohit Kumar
  - Shubham Kumar
  - Zaheer Timol
+  - GeongiMoon


Needs a space, I am assuming?

If not let me know, other than that looks good.

I missed a space... Thank you!

hmacdope · 2023-08-16T06:19:52Z

testsuite/MDAnalysisTests/topology/test_guessers.py

@@ -104,6 +104,9 @@ def test_guess_atom_element_1H(self):
        ('zn', 'ZN'),
        ('Ca2+', 'CA'),
        ('CA', 'C'),
+        ('N0A', 'N'),


One more nit sorry could you test Na+ -> Na?

I just added two tests for Na+ and Cu2+

IAlibay

The one question from me.

IAlibay · 2023-08-16T09:17:20Z

package/MDAnalysis/topology/guessers.py

+        no_numbers = re.split(NUMBERS, no_symbols)
+        no_numbers = list(filter(None, no_numbers)) #remove ''
+        # if no_numbers is not empty, use the first element of no_numbers
+        name = list(filter(None, no_numbers))[0].upper() if no_numbers else ''


Apologies if I'm missing something obvious, but do you need to apply filter again here? You've already removed the '' entries and you shouldn't have any None entries ?

Sorry, I think I made a mistake while modifying the code...

remove unnecessary code

IAlibay

Thanks, sorry for this taking so long. lgtm!

hmacdope

LGTM also thanks @pillose

hmacdope · 2023-08-16T12:28:55Z

Cycling CI

github-actions bot added the Component-Topology label Jun 14, 2023

github-actions bot reviewed Jun 14, 2023

View reviewed changes

pillose force-pushed the develop branch from 6ea0049 to 342d9a8 Compare June 14, 2023 14:24

modified guess_atom_element for more accurate guess

921616b

pillose force-pushed the develop branch from cbce66c to 921616b Compare June 14, 2023 15:22

hmacdope requested changes Jun 26, 2023

View reviewed changes

IAlibay added the close? Evaluate if issue/PR is stale and can be closed. label Jul 29, 2023

pillose and others added 4 commits July 30, 2023 05:50

Merge branch 'MDAnalysis:develop' into develop

e358dcb

add test for new behavior

da13425

Merge branch 'develop' of github.com:pillose/mdanalysis into develop

80c8eb2

updated AUTHORS and CHANGELOG

1ce5387

hmacdope requested changes Jul 30, 2023

View reviewed changes

IAlibay requested changes Jul 31, 2023

View reviewed changes

IAlibay removed the close? Evaluate if issue/PR is stale and can be closed. label Jul 31, 2023

pillose closed this Aug 1, 2023

pillose force-pushed the develop branch 2 times, most recently from 1ce5387 to 66a2b21 Compare August 1, 2023 10:44

github-actions bot removed the Component-Topology label Aug 1, 2023

pillose added 4 commits August 1, 2023 19:50

revised the order of CHANGELOG

76d6bec

add commnets and modified the code a little

aa7f457

Merge branch 'develop' of github.com:pillose/mdanalysis into develop

878b496

add my username

35abfaa

pillose reopened this Aug 1, 2023

github-actions bot added Component-Topology labels Aug 1, 2023

RMeli reviewed Aug 12, 2023

View reviewed changes

package/CHANGELOG Outdated Show resolved Hide resolved

package/AUTHORS Outdated Show resolved Hide resolved

package/MDAnalysis/topology/guessers.py Outdated Show resolved Hide resolved

pillose and others added 4 commits August 15, 2023 23:12

Update package/CHANGELOG

3454ceb

Co-authored-by: Rocco Meli <[email protected]>

Update package/AUTHORS

7b5285a

Co-authored-by: Rocco Meli <[email protected]>

Update package/MDAnalysis/topology/guessers.py

ed4bbb4

Co-authored-by: Rocco Meli <[email protected]>

Merge branch 'develop' of github.com:pillose/mdanalysis into develop

d1d4f54

pillose requested review from hmacdope and IAlibay August 16, 2023 05:37

Merge branch 'develop' into develop

075f2dc

hmacdope requested changes Aug 16, 2023

View reviewed changes

Update AUTHORS

f77c6f5

hmacdope requested changes Aug 16, 2023

View reviewed changes

pillose and others added 2 commits August 16, 2023 17:43

add guesser tests

5fe84ab

Update test_guessers.py

9d30a1e

IAlibay requested changes Aug 16, 2023

View reviewed changes

Update guessers.py

575e6eb

remove unnecessary code

IAlibay approved these changes Aug 16, 2023

View reviewed changes

hmacdope approved these changes Aug 16, 2023

View reviewed changes

Merge branch 'develop' into develop

19511fc

IAlibay merged commit acfd122 into MDAnalysis:develop Aug 16, 2023

orbeckst mentioned this pull request Aug 29, 2023

initialize guess_atoms function PR Becksteinlab/MDPOW#264

Merged

IAlibay added the enhancement label Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modified guess_atom_element for more accurate guess #4168

modified guess_atom_element for more accurate guess #4168

pillose commented Jun 14, 2023 •

edited

Loading

github-actions bot left a comment

github-actions bot commented Jun 14, 2023 •

edited

Loading

codecov bot commented Jun 14, 2023 •

edited

Loading

hmacdope left a comment

IAlibay commented Jul 29, 2023

pillose commented Jul 29, 2023

hmacdope left a comment

hmacdope Jul 30, 2023

hmacdope Jul 30, 2023

IAlibay left a comment

RMeli left a comment

pillose commented Aug 15, 2023

hmacdope Aug 16, 2023 •

edited

Loading

hmacdope Aug 16, 2023

pillose Aug 16, 2023

hmacdope Aug 16, 2023

pillose Aug 16, 2023

IAlibay left a comment

IAlibay Aug 16, 2023

pillose Aug 16, 2023

IAlibay left a comment

hmacdope left a comment

hmacdope commented Aug 16, 2023

modified guess_atom_element for more accurate guess #4168

modified guess_atom_element for more accurate guess #4168

Conversation

pillose commented Jun 14, 2023 • edited Loading

PR Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 14, 2023 • edited Loading

Linter Bot Results:

codecov bot commented Jun 14, 2023 • edited Loading

Codecov Report

hmacdope left a comment

Choose a reason for hiding this comment

IAlibay commented Jul 29, 2023

pillose commented Jul 29, 2023

hmacdope left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

RMeli left a comment

Choose a reason for hiding this comment

pillose commented Aug 15, 2023

hmacdope Aug 16, 2023 • edited Loading

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

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope commented Aug 16, 2023

pillose commented Jun 14, 2023 •

edited

Loading

github-actions bot commented Jun 14, 2023 •

edited

Loading

codecov bot commented Jun 14, 2023 •

edited

Loading

hmacdope Aug 16, 2023 •

edited

Loading