NeuralEnsemble / python-neo

Neo is a package for representing electrophysiology data in Python, together with support for reading a wide range of neurophysiology file formats
http://neo.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
316 stars 249 forks source link

Codejam #6 :Implement all attributes as properties, with built-in compliance testing. #128

Closed samuelgarcia closed 10 years ago

samuelgarcia commented 10 years ago

Implement all attributes as properties, with built-in compliance testing.

Place to discuss.

toddrjen commented 10 years ago

Basically, each existing recommended or requires attribute would have a property with setters and getters. Inside the setter would be tests to make sure the data is valid, so assert_neo_object_is_compliant would be removed.

samuelgarcia commented 10 years ago

Will this impact IO ?

toddrjen commented 10 years ago

since the properties will wrap hidden attributes, the testing can be bypassed in performance-critical situations or in cases where not all necessary information is available.

for these situations, it would probably be best to keep a private _is_compliant or _assert_compliant method to do a check at the end.

rproepp commented 10 years ago

Since we're all consenting adults, I wonder if we need this at all? Is improper usage of attributes a real problem?

toddrjen commented 10 years ago

This wouldn't just throw an error if someone does things a little bit wrong. For example, if you provide a "right_sweep" value to spike_train without units, it can automatically add the unit from the spike times. I suspect in most cases, this sort of sane automatic handling can be done. Throwing errors would only happen when the automatic processes can't handle it.

Or if you have a property that expects an array, and you provide a scalar, it can automatically convert it to an array for you.

So I think this would provide a benefit to users in most cases. They don't need to manually specify every little thing to make sure something doesn't break. The properties would know more or less what they need, and would take care of obvious housekeeping themselves.

rproepp commented 10 years ago

That seems too magical for me. Automatic guessing of units is dangerous and can lead to annoying errors. Checking for mistakes is fine with me, but I don't know if it is necessary. Adding implicit helpers to the properties goes too far in my opinion, and seems to contradict a few of the Zen "commandments"...

toddrjen commented 10 years ago

So is it agreed that this is not a good idea?

apdavison commented 10 years ago

I think we should review this on a case-by-case basis. For some attributes, a property may be the best approach (e.g. Block.segments), for others the consenting-adults principle should apply.

toddrjen commented 10 years ago

Yes, that is definitely true. We already do that in some places. But it would not be a general rule, correct?

apdavison commented 10 years ago

Agreed, no general rule either way.