Open richardjgowers opened 4 years ago
@richardjgowers i would like to work on this issue
I would like to work on this issue
@richardjgowers sir, can you please review my Pull Request #2788 regarding this issue
@richardjgowers is this issue still relevant? Is there anything where you'd want to describe more clearly what you'd want to see so that the issue becomes suitable for the GSOC-starter label?
is this issue still open to work on
I don't see an open PR referencing this issue so please feel free to open a PR. Make sure that you fill in the PR comment template properly and reference this issue.
Hello @orbeckst. I'm Fortune from the Outreachy program. I just opened a pull request for this issue.
@richardjgowers I'm not sure you can blindly enforce dtypes without also giving a preferred actual type; the dtype of string arrays is set to object
, which an integer also is. Other people have also wanted to set TopologyAttrs of arbitrary type as well, so we can't assume every object is a string. The fix for strings might need to edit _StringInternerMixin._set_X
to just warn or error if a non-string is given.
The TypeError raised in the initial message isn't just because the dtype is wrong, it's because the value setter assumes the given value is either a string or a list. Then no further type checking is done. Setting multiple names works fine:
>>> u.atoms[:3].names = [1, 2, 3]
>>> u.atoms[:3].names
array([1, 2, 3], dtype=object)
Tagging @fortune-max here as well as you've opened a PR for the issue :)
Ok I see, its possible there are object topologyattrs that aren’t string, so hacking the internet mixin seems good yeah.
On Sat, Apr 23, 2022 at 09:19, Lily Wang @.***> wrote:
@richardjgowers https://github.com/richardjgowers I'm not sure you can blindly enforce dtypes without also giving a preferred actual type; the dtype of string arrays is set to object, which an integer also is. Other people have also wanted to set TopologyAttrs of arbitrary type as well, so we can't assume every object is a string. The fix for strings might need to edit _StringInternerMixin._set_X to just warn or error if a non-string is given.
The TypeError raised in the initial message isn't just because the dtype is wrong, it's because the value setter assumes the given value is either a string or a list. Setting multiple names works fine:
u.atoms[:3].names = [1, 2, 3] u.atoms[:3].names array([1, 2, 3], dtype=object)
Tagging @fortune-max https://github.com/fortune-max here as well as you've opened a PR for the issue :)
— Reply to this email directly, view it on GitHub https://github.com/MDAnalysis/mdanalysis/issues/2764#issuecomment-1107428175, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACGSGBYL4DO2ZGUG322OVC3VGOXBHANCNFSM4N7PQFRA . You are receiving this because you were mentioned.Message ID: @.***>
@richardjgowers I'm not sure you can blindly enforce dtypes without also giving a preferred actual type; the dtype of string arrays is set to
object
, which an integer also is. Other people have also wanted to set TopologyAttrs of arbitrary type as well, so we can't assume every object is a string. The fix for strings might need to edit_StringInternerMixin._set_X
to just warn or error if a non-string is given.The TypeError raised in the initial message isn't just because the dtype is wrong, it's because the value setter assumes the given value is either a string or a list. Then no further type checking is done. Setting multiple names works fine:
>>> u.atoms[:3].names = [1, 2, 3] >>> u.atoms[:3].names array([1, 2, 3], dtype=object)
Tagging @fortune-max here as well as you've opened a PR for the issue :)
Okay, nice. A warning would definitely be better. What about the issue of assigning floats to ids as stated above, should that also throw a warning? @richardjgowers
@richardjgowers @lilyminium I have gone through the issue description, raised PRs, conversation and from that i understand this issue as to update the _set_X
(located here) such that before setting the values we first make sure that the values are of specific type (as requirement).
_set_X
is called from this classes:
AtomStringAttr
(located here)ResidueStringAttr
(located here)SegmentStringAttr
(located here)_set_X
function such that it will check for the required dtype (dynamically with passed dtype to the function).def _set_X(self, ag, values, required_dtype):
if not isinstance(values, required_dtype) and not all(isinstance(v, required_dtype) for v in values):
raise TypeError(f"Values must be of type {required_dtype.__name__}")
_set_X
from above three classes then we pass the required dtype there like shown below.class AtomStringAttr(_StringInternerMixin, AtomAttr):
@_check_length
def set_atoms(self, ag, values):
return self._set_X(ag, values, str)
@HeetVekariya you've got the string part of it very close! The overall issue here is that TopologyAttrs can have different types (which is set by the dtype
property of each subclass, e.g. https://github.com/MDAnalysis/mdanalysis/blob/develop/package/MDAnalysis/core/topologyattrs.py#L540 )
When setting the attribute, we should check that the values match this dtype and raise an error if not. Right now for example, as mentioned in the top comment, setting resid
to a float silently converts it to an integer, whereas we probably just want to raise an error instead. So a fix to this issue should work for all cases, i.e. in both the integer/float/etc dtypes, and the string dtypes (more on that below).
My comment (https://github.com/MDAnalysis/mdanalysis/issues/2764#issuecomment-1107428175) is referencing that some TopologyAttrs (e.g. Resnames https://github.com/MDAnalysis/mdanalysis/blob/develop/package/MDAnalysis/core/topologyattrs.py#L2738 ) have dtype
object
, but are supposed to be strings. Checking if basically anything is an object
in Python will return True, so using an isinstance
check would be quite ineffective with these string types. Luckily, these string TopologyAttrs are all subclasses of _StringInternerMixin
, which uses the ._set_X
, so my comment was suggesting that the .set_X
method be modified. Since we know that _StringInternerMixin
should have strings, therefore we already know that required_dtype
argument you've suggested in your solution should be a string, and it's not necessary to pass that in.
Even in the other non-string classes, required_dtype
should not be necessary since each class should have a dtype
property containing this information already.
Also do i need to write any test cases for this functionality if we proceed with methodology.
Yes, this change should be tested thoroughly with separate cases for different types!
@lilyminium Thank you for guiding me.
This is the updated approach,
def _set_X(self, ag, values):
# Type checking logic
if self.dtype is object: # Special handling for string attributes
if not (isinstance(values, str) or all(isinstance(v, str) for v in values)):
raise TypeError("All values must be strings for string attributes")
else:
# Handling for non-string attributes
if not (isinstance(values, self.dtype) or all(isinstance(v, expected_dtype) for v in values)):
raise TypeError(f"All values must be of type {self.dtype.__name__}")
- Uses class variable ```dtype```, rather than passing argument as ```required_dtype```
- ```Object``` type is handled explicitly to check for string
- Sorry for asking question like this without creating PR 🙏
### Question:
1. For all three classes mentioned above, does not contains any ```dtype``` variables in it, so how it will work ? Does ```dtype``` for that classes is defined in the inherited classes of them ?
2. How can i test my updated code in local for these changes ?
Sorry for asking question like this without creating PR
No worries @HeetVekariya, it's better to have a clear approach and know what you want to do before creating PRs!
dtype
in those classes, although subclasses do. I think any solution here can just assume that anything subclassing _StringInternerMixin
should be a str
. Do keep in mind that _set_X
is only in the _StringInternerMixin
class and all other non-string TopologyAttr
classes would probably have a different solution, likely something to do with set_atoms
, set_residues
, set_segments
etc.conda
, which can be very slow).No worries @HeetVekariya, it's better to have a clear approach and know what you want to do before creating PRs!
Thank you 🙏 :smile:
_set_X
is from the class which inherits _StringInternerMixin
class.
def _set_X(self, ag, values):
if not isinstance(values, str) and not all(isinstance(v, str) for v in values):
raise TypeError("All values must be a string")
set_atoms
, set_residues
and set_segments
, i want to ask if you are talking about methods defined in the above mentioned classes ( set_atoms, set_residues, set_segments ) which makes function call to _set_X
or which sets the values like set_atoms, set_residues, set_segments ?
_set_X
then,
_StringInternerMixin
, so all of them would be having str
dtype str
then we can pass required/expected dtype
to _set_X
as argument, which will handle it.@HeetVekariya the reason I bring up set_atoms
, set_residues
, etc. -- this issue is about all TopologyAttrs defined in topologyattrs.py. Not all of them subclass _StringInternerMixin
and not all of them have string dtypes. Therefore, while your solution where you modify _StringInternerMixin._set_X
will work for the string TopologyAttrs (Atomnames, etc) and is part of the solution, it won't work for all TopologyAttrs (Resids, etc).
Perhaps the easiest way for you to see what I mean is to write some tests that currently fail, but will pass with the right solution. For example, try the examples listed in the top comment https://github.com/MDAnalysis/mdanalysis/issues/2764#issue-639557421. In both those cases, the code @richardjgowers has given, should produce an error. Perhaps you could use pytest.raises to write a test that expects a particular error to be raised in those scenarios. There are examples of similar tests in the testsuite, e.g. here and here.
Writing tests should pass with your solution, but currently fail, is a great first step in opening a PR as it allows you to check that you're actually fixing the right behaviours -- it's basically test driven development.
@HeetVekariya yes sure, go ahead.
@lilyminium Hi there ! Hope you are doing well.
Thank you for your guidance till now.
@HeetVekariya as @lilyminium said above: start a PR where you write a test that fails. Then go from there. The PR can be small and doesn’t have to solve the problem. But it’s a lot easier for others to comment on existing code than to talk about solutions in theory.
You already have a number of code contributions and are more experienced. The kind of comments that you got above are typically what we would consider sufficient for a developer to get started on a problem.
Our Topology system is statically typed, each
TopologyAttr
(found here) should only accept one data type, e.g. atom names (here) should always be strings, resids should always be ints.Currently setting an atom name to an integer gives "
TypeError: 'int' object is not iterable
"This should instead fail and inform that atom names need to be strings.
Setting a Residue's resid to a float seems to work...
This probably is using the inbuilt rounding in numpy int arrays, but it should probably raise an error too.
To fix this, we could have a
check_dtype
decorator aroundTopologyAttr.set_atoms
(and residue/segment) to check that the supplied values are the correct type.