Improvements to NDData class - #348

I've now added some basic 'getting started' documentation for the new functionality

This code is a great implementation of the decisions made yesterday during the telecon and should be merged so the NDData children classes have a common API to build from.

Nice job @astrofrog -- this looks great!

I've tweaked the implementation of the error propagation very slightly. Before, methods such as propagate_add took the operand and a handle to the final NDData object. This assumes that the data attribute has already been updated prior to being passed, but of course there was no guarantee of this being true, and it's not really the final object since it's still incomplete by then (e.g. masks and flags aren't set). In addition, I was returning result_error which gets set outside propagate_add, so you had:

def propagate_add(self, operand, result):
    ...
    return result_error

result.error = e.propagate_add(operand, result)

which seems strange because it could have just set the error in-place.

The reason propagate_add takes the result is because the final value from the operation is needed for e.g. multiplication and division. But it doesn't really need anything else than the modified data (because the flags, masks, etc. haven't been set yet) so it's easier to just pass the data object:

def propagate_add(self, operand, result_data):
    ...
    return result_error

result.error = e.propagate_add(operand, result.data)

which is more consistent and elegant in my view.

I've added detection of correlated errors as discussed on the mailing list. Here's an example in action:

https://gist.github.com/3413356

As part of this, I added a mechanism to disable error propagation (propagate_errors=False).

Docs and tests are still needed for this.

EDIT: this has since been removed for now

@eteq - could you review this once you get a chance? (no rush though).

@astrofrog - will do in a day or two

I have rebased, and removed the code to detect correlated errors, replacing it instead with warnings in the documentation about correlated errors not being supported.

This is now ready for a final review.

One general comment on the documentation: Right now it's a bit confusing, because all of the information is in "getting started", and there's not really anything about the maining NDData classes in the "using" section. So perhaps add a "Subclassing nddata" page to the "using" section? Or maybe just an "NDData overview" that has some of the information now in "getting started"? In the latter case, you could then shorten "getting started" by removing the section headings and have it be mostly just an evolving example that adds each of masks, flags, and errors, in turn.

Other than that and my inline comments above, looks great to me!

Now that the 'getting stated' section has grown, I agree we should move some of it to the main docs section. I'll work on that this week.

@eteq - I've tried addressing all the comments you raised. A couple of points - regarding your suggestion of:

operand -> other_nddata
result_data -> result_array (or result_arr)
self.parent -> self.nddata_object (or maybe self.parent_nddata)

I changed operand to other_nddata and self.parent to self.parent_nddata, but I want to leave result_data the same because it goes better with result_error.

Regarding:

class MyVarError(NDError):
    def propogate_add(self, operand, result_data):
        lefterror = self.parent.error.array
        righterror = other_nddata.error.array
        result_array = result_data.array

        return (lefterror + righterror)/resultvalue

the line:

result_array = result_data.array

is actually incorrect - result_array or result_data is actually an array, so shouldn't take the .array. In addition, the methods should return a class instance, not an array, so

return (lefterror + righterror)/resultvalue

is also not quite right.

I've significantly re-arranged the docs and added a section about sub-classing that discusses all this. Could you take a look over it now?

Regarding sub-classing errors, I think we may decide to change the API slightly when we actually end up implementing another error class, so we shouldn't consider it set in stone.

@astrofrog - Re the name changes: my concern over result_data is that when I first looked at that name, I thought that was the result NDData object - that's what the data part sounds like to me. Hence, that lead to the confusion that you've now clarified. And actually, I'm still confused: is result_data an NDData object, or a numpy array? (also, see my inline comments about the docs).

The rest looks great, though!

I think I've addressed all your comments. Regarding result_data, I've made it clearer it should be a Numpy array.

I think that the NDError API could definitely be improved in future - for example, one could imagine that in some cases how the data is combined will depend on the error propagation, so maybe the NDError sub-classes should actually do the data arithmetic too (e.g. if the data represents the mean of a PDF, then when two PDFs are combined the data may change non-trivially). In that case, we just need to pass other_nddata to propagate_add and it would return result_nddata, an NDData object. But better is the enemy of good, so I suggest we go with this for now, and try and think of how to make it cleaner and more robust. Maybe we'll just have to wait until other error classes are implemented to see if any issues pop up.

Anything else that needs to be addressed before merging?

Merge ahoy!

Sorry to leave you hanging here - I'm happy with this as-is, and as far as I can tell everyone else is, so I'll go ahead and merge it right now!