Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

change value related behaviors #237

Merged
merged 24 commits into from
Mar 15, 2018
Merged

change value related behaviors #237

merged 24 commits into from
Mar 15, 2018

Conversation

jgrewe
Copy link
Member

@jgrewe jgrewe commented Mar 8, 2018

replacement pr for #236. The following changes have been made:

  1. prop.value returns a copy(!) of the value list. Manipulating this list will not affect the content of the property (fixes Usage of python list as odml value circumvents dtype checks #227).
  2. implement prop.append, prop.extend methods. The first only for single convertible values, the latter for lists of such as well as other properties. (fixes add append value functionality for properties is verison 1.4 #223)
  3. Implement the __setitem__ method for direct value manipulation. Property more or less behaves like a list. Passed values are checked for converability.
  4. Implement prop.merge method. This will sync all but the dependency and dependencyValue attributes. ValueErrors are raised, if information is set in both properties but are in conflict. (fixes Add merge functionality on value level #221)
  5. add some docstrings
  6. some tests

@coveralls
Copy link

coveralls commented Mar 8, 2018

Coverage Status

Coverage increased (+0.4%) to 71.328% when pulling 4667b18 on jgrewe:property into 2830914 on G-Node:master.

Copy link
Contributor

@mpsonntag mpsonntag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really nice to use now!

[1, 2, 3]
>> p.value.append(4)
>> print(p.value)
[1, 2, 3]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line should be [1, 2, 3, 4] ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hehe, no, I think this is correct, p.append(4) would change it, p.value.append(4) appends to the copy and not the property

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argh sorry, blanked out the .value. part...

odml/property.py Outdated
"""
# Make sure boolean value 'False' gets through as well...
if new_value is None or (isinstance(new_value, (list, tuple, str)) and len(new_value) == 0):
self._value = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Property.dtype is currently not reset when resetting the values. Maybe this can be reset here as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do have the dtype setter for this. Imho, I would rather not reset it on deleting the value(s)

@JuliaSprenger
Copy link
Contributor

In general I am happy with this implementation, there are just a few cases, where I think the behaviour should be different

  • providing a generator as value should list the generated items, not the string version of the generator (eg odml.Property(name='prop', value=range(10)) should contain a list of numbers, not a string)
  • The automatic dtype conversion should not introduce a loss of information
    prop1 = odml.Property(name='prop1', value=[3])
    prop2 = odml.Property(name='prop2', value=[3.5])
    prop1.merge(prop2)

should result in an Error rather than prop1.value == [3,3]

Also I suggest to also add a section.extend() function similar to the property.extend() function, buts that's probably a different issue.

@jgrewe
Copy link
Member Author

jgrewe commented Mar 14, 2018

@JuliaSprenger you can now pass a range or anything that defines __next__ or __iter__ as value.
Further, merge. extend and append got a strict option that by default raises ValueErrors if datatypes do not match. If strict is False there will be the implicit conversion with possible loss of information.

@JuliaSprenger
Copy link
Contributor

The strict option is a good idea, but I think it should be also available for section.merge and from there on be passed on to all merges of subsections and properties.

@jgrewe
Copy link
Member Author

jgrewe commented Mar 15, 2018

yes, agreed, would you mind to add it to issue #246, which was opened by Michi for the section append/extend/merge point you made

@JuliaSprenger
Copy link
Contributor

Cool! I added a comment to the other issue. Then I think we can merge now.

@JuliaSprenger JuliaSprenger merged commit 962f661 into G-Node:master Mar 15, 2018
@jgrewe jgrewe deleted the property branch March 15, 2018 13:14
mpsonntag added a commit to mpsonntag/python-odml that referenced this pull request Mar 26, 2018
mpsonntag added a commit to mpsonntag/python-odml that referenced this pull request Mar 27, 2018
@mpsonntag mpsonntag mentioned this pull request Mar 27, 2018
@JuliaSprenger JuliaSprenger mentioned this pull request Mar 28, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
4 participants