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
Add in analysis function to calculate MSDs #2619
Add in analysis function to calculate MSDs #2619
Changes from 66 commits
826b2ad
6540de2
f50622c
aa29d3b
6e72bd4
671e537
b0e296b
67fc0d0
3018a62
44407e4
4711fb6
f9f7dd9
2358a11
a8d400a
0a960e4
cc11cc6
e9b17fe
09b5458
a06294b
4cf63fd
e5e3629
2c469f0
546f7f2
e02dd09
033273b
dd5b25d
83be862
d318a91
10e3fc2
089a2bd
d108453
8ebc233
e2d383d
73bc1f7
c71f5e6
ddbc4e3
523f81f
6eb76e2
86d59bb
a934882
7759da7
851daa3
d23ee0e
49fbd86
9af111a
19625f0
0f637dd
4c5f487
78f7d4f
b458ae8
09416cb
60f0cca
38da36e
0e22adf
504e19c
e805403
3ba54a0
6f70f06
4894bbd
3a48b61
0d34487
4ca2bf7
8249a65
63b8fff
0fc07f7
0d69b65
98d4b48
7d530eb
2c760eb
a7136c0
ef6b6f4
f4aeaea
121ac17
09bedac
e070fb8
d8c81cd
1ac4086
7a58d73
c3ae32b
7764f9e
9005ad5
e898226
b370a27
3f5f5f3
e4e46ba
d09f79c
cff23b0
ba6f37a
42720d8
0f5a009
cc53e1b
79f93a4
e9f4cf4
b909aad
bac625b
1aef299
2648d05
a5a9716
eccc5ea
f27b0da
33a8907
fb7f458
ac3a525
5e8c17f
41dde3f
269ea05
ae5b93b
2feb81e
141dca8
d007ac9
e1dbbbe
923e161
607e228
e983f83
7d4e54f
8498f1a
e39433e
50b9f11
af5cedf
e76f164
1d62c0c
a55ad30
491da98
09fb909
4d9c2d5
0cf1882
f1c7bd2
7321bb6
18eb09c
353ae3e
78911b8
e8478ba
6513f8b
38eeb15
fe02375
16dca64
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.
@orbeckst do we have a consensus on what the default
select
s should be? e.g. 'name CA' as a sensible default for memory-hungry analyses like this one?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 feel 'name CA' would be confusing as a default? Trying to find the diffusivity of a set of backbone atoms is from what I can tell rare.
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 agree with @hmacdope that CA does not make sense here.
Can
u
also be aAtomGroup
? If so,select=None
should select everything so that one can easily use AtomGroups that already contain what's needed.If
u
can only be aUniverse
then I am tempted to makeselect
a required argument to force the user to say exactly what they want. Analyses classes are free to implement any call signature that they like (see the example for NewAnalysis).NewAnalysis(atomgroup, **kwargs)
andNewAnalysis(universe, select=None, **kwargs)
are common butNewAnalysis(universe, select, **kwargs)
is allowed.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.
As it currently stands can only be a
Universe
. I will changeselect
to be required as suggested?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.
select
no longer a kwargThere 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.
None of your code requires a Universe, you could just make it accept an
AtomGroup
by initialising withu.universe.trajectory
. I think that's really it.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 it typical to have
select=None
select all the atoms instead of directly settingselect='all'
as the default?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 honestly don't remember.... there's been something in the select vs selection discussion, hasn't it? In terms of "pythonic" behavior I always expect
None
for a kwarg to give me the default as opposed to the semantic "nothing". You're right, though, that in this case one could be clearer and set the default to"all"
. (u.select_atoms(None)
gives an empty AtomGroup so I don't think that lettingNone
pass through is the right approach.)