Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
basic guesser features #3753
basic guesser features #3753
Changes from all commits
60b24c1
ce68bd1
ae862fc
c5d3453
f6053dc
bb04030
91f0d08
2a013f1
93ffc3f
bf41f72
9cd3460
ca1f871
6d48520
4f2782f
6661c36
8b0cec5
5a9ed58
b28f09a
6fe79ac
fc4ec30
ede16ca
76275a3
f7bbbe6
f1a09bb
4dc0ada
770b4ce
2c92aa0
5125953
b0a0ee2
c2f1f9a
6b2c4c7
0c9e78d
bf2463f
c159461
2039be7
6b6c07b
2ceeec6
4679c18
5f81d3e
60cedfc
a7ea8da
18c86f8
3d996c4
23932ca
1d988de
75e7b81
baa3e0b
54b2a64
c85a593
2bd7fff
81c5382
78365bd
97a3d6b
c796eee
63d6fe1
8748515
c010d47
b6fa523
ba3e733
3711962
10b8c03
c0f826f
636e4bc
752f05e
d0e2a8a
986335e
b80edec
438ac52
5d6772c
0dcca86
1a74cfd
eba975c
45f90ef
ca525ad
05a7623
9318bf2
0297954
2c88dd0
d1516d0
99c8684
0f4fb3d
0d999f1
c6431c0
1e4395c
23d627b
d77e14f
ba1d392
8aaa305
b120594
0901cb3
c9dfa0b
72e62f0
d2f0ec6
3ba87de
6a819b9
f3669df
044bc74
4a821e1
ed05187
1032019
7b0041c
2fe59fa
5d6550e
6a1e4c5
25aaf73
501e397
d9a65a7
4395f6f
c234e75
fdb296c
c4b200d
a9b4354
965cd72
6338a8e
4324f72
17fded8
4c29a39
9fa7b32
f450c6f
f6b9b97
460d2f5
af84927
002e1d6
390ae37
a5c303c
b01b092
61831f8
7f638fb
cd37c19
71e9a2b
084ab13
fa9128a
991070c
62c43cd
314b31c
0dcbacd
9a230a4
f82bdcf
6f6f634
40ef1ed
9222d96
b712a77
8724bba
0abb069
c1b05ba
02d62b1
710df7d
d7cb653
5f6a6f3
f12e5f1
d76362b
a8ba36c
49c803c
f816441
e1347a7
3ebce3f
10cad41
626cb39
b476301
3fbdc46
7b42095
21aae8e
3e5a9e9
98d91fe
78654e5
a954aee
dfb3761
3df1d9b
e273d68
4ad3948
1518729
d1e27e3
348f62d
e2c03cd
ad50c17
7663a5b
aece5bc
6175b37
52fcb41
a463561
d67c921
492ccd7
f50a7b1
3dc7535
2e8d863
a22afc0
0285dd1
476444e
3c3df98
6be3ec4
7b0ce2e
aaf1d08
1fd7d69
e29d3e6
f64ca1d
c0ff781
58a0d75
2291a4d
21950f6
e7c0241
c0ce6e0
b776f09
1d8e2cd
fc18d4f
06211cc
240af1c
ff71b1a
2aeb0fc
7580855
202d62a
27fcf3b
5c5c684
5ccd1d1
b343d11
b3f292d
8dc935b
51c75a6
e1e5b3e
68580ea
6da8ca8
5d505a4
934d7e6
c7ca389
14a6ed2
bd12db2
88bb017
40c7632
28aeb6a
da15709
83ee7f5
c67dc3a
ca43651
67ad49e
5ee7bae
817cab1
6f60435
a946156
ba9deaf
fc0ebe6
4976166
77fb208
eeaf35a
80a1b74
8986a97
c0b833e
00dff7c
1c15f21
7cc0341
c225f75
e7ddaaf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How are we transplanting this back in? This behaviour can't change for the 2.x release of MDA.
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.
all the guessing behavior is transferred to the universe through the
guess_topologyAttribute
API without breaking default behaviorThere 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.
To double check, we are removing this note because the default behaviour is equivalent to this note?
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.
yes indeed... the idea is this is no longer specific/happening inside the parser
Check warning on line 184 in package/MDAnalysis/converters/OpenMMParser.py
Codecov / codecov/patch
package/MDAnalysis/converters/OpenMMParser.py#L184
Check warning on line 198 in package/MDAnalysis/converters/OpenMMParser.py
Codecov / codecov/patch
package/MDAnalysis/converters/OpenMMParser.py#L198
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 there a reason the types used to be guessed rather than just assigned
elements
? The change looks reasonable to me, but I am worried there is a subtlety we may be missing here.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 removed all type guessing from parsers and transferred it to be guessed at the universe creation. For PDB, FIAMS, XYZP and RDKit parsers, I assigned the
atomtypes
to be either read from the file if not be assigned to elements, if not be assigned to names (as per issue #2931). but if we are not ready for this move I'll remove it no worriesThere 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 is OK. I just wanted to avoid missing something. We should keep note of that and maybe have it a separate entry in the changelog.
We will also need to be careful with the tests.
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.
To what extent does this change behaviour? Does this amount to a breaking change?
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 haven't checked the test for that yet. Hopefully, it should not be a problem, except... The one case I see that could break is if the type was guessed as
CA
because we historically have an alpha carbon type.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.
This does not change the types that were guessed, the "CA" is guessed as a "C" in both
develop
and this PR:u2.atoms.types
andelements
is "C",u2.atoms.names
stays "CA".Check warning on line 309 in package/MDAnalysis/converters/RDKitParser.py
Codecov / codecov/patch
package/MDAnalysis/converters/RDKitParser.py#L309
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.
@aya9aladdin @lilyminium - this is assigned but nothing else is done with it afterwards it seems? I feel like I'm missing something, could you please just double check?