Tidied up mask, flags, error - #303

Just seeing this discussion now.

@wkerzendorf - I asked about storing masked arrays in #266. This was @eteq's reply:

we definitely don't want data to be a masked array, because numpy masked arrays have significant performance
penalties. Also, they are more limited in what "mask" means than what we intend (in the future) for NDData.

I agree that masks and flags should not be broadcast, for the reasons that @astrofrog mentioned. For errors, definitely agree we want to allow things like scalars. My question is, should they be broadcast upon construction of the NDData object? It makes using an NDData object simpler: one always knows what to expect error to be. Right now if you pass the constructor a scalar, error ends up as a 0-d array, which I think is not what we want.

Note that in the above comment I posted from @eteq I think he was still thinking about masks as a combined mask + flag object, not what we are thinking now (separating mask and flag attributes). But the part about performance still stands.

@kbarbary: I think we want masks to be the same shape as the array. But why not let flags fly free, and do whatever they want as long as they can be broadcasted. Who knows what cool features future astropy users can come up with and it doesn't hurt the performance or anything else for that matter.

we can't broadcast errors on construction as they are first of all their own object (remember they are not necessarily an array) and if they are higher dimensional than nddata they can't be broadcast in that sense at all.

Finally, I find a 0-d array a aesthetically and mathematically pleasing representation of a scalar (but I'm weird I know ;-) ).

@wkerzendorf

Regarding errors: You're right, looking at this code I was thinking about errors as ndarrays still. I forgot that we are heading towards error classes. This makes the discussion of 0-d arrays irrelevant.

Regarding flags: I see flags and masks as very similar. Both are basically metadata on each datapoint. Masks are "should I use this datapoint?" and flags are "what are all the other attributes of this datapoint?" I don't see why they should be treated differently, and would find it confusing and unnatural to do so, unless there is a specific use case in mind.

I can only think of flagging entire columns of data more easily, but then, why wouldn't we allow this for masks as well? The inconsistency leads to confusion for the user: Say a user has a 2-d data array, and a 1-d flags array (broadcastable to the 2-d data). The user wants to mask everything with flags == 2, so (very reasonably) sets mask = (flags == 2), then tries to initialize the NDData object. This would be a fine input for flags, but would fail for mask, since it's 1-d.

All right, I give up :-) flags can be broadcastable.

@eteq - any objections to merging this in?

I just realized, we don't need to care about broadcasting or not as flags are objects like errors. Are masks objects?

Why do flags need to be objects? Don't we have enough flexibility if we allow them to be numpy arrays of any type?

@kbarbary - I still agree that flags should map directly to the data, so if someone else weighs in then we can make a final decision (for now...)

well not really, I mean if flags can be anything then how does arithmetic, convolution interpolation work. I would just make flags error like objects. Then we don't have to deal with things.

Ok, well we can decide that once we actually get to the point where we have a decent implementation of a Flag class, but for now I'll leave them as Numpy arrays.

Masks are definitely only boolean arrays - that was the whole point of the separation of masks and flags.

I agree: for now we should just leave flags as ndarrays (as we are doing with errors!). I think the point of this PR is just to separate flags and masks. It is not meant as a final version.

So thinking about this a little bit more: as we said in the beginning: flags/masks are essentially the same. I would say that masks are a certain type of flags.

As this special type of flag is used most often, we have provided an easy interface to use them. And as we're on a most used case we have also decided (I agree with you now there) that masks are simple boolean arrays of the same shape as the value array.

However, not in all use cases that makes sense - so special masks can just be implemented through the flags. I would put a simple flag class up now that just has internally numpy arrays that get added, subtracted, divided and multiplied just as their parent nddata object does.

Anyways, we agree: values and masks are numpy arrays with the same shape.

I would put errors and flags as objects, and I can do a pull request on that in the next few days.
I mean pull request with a very basic flag class.

I think we're mostly done here. I vote for merge as far as I can see. Thanks @astrofrog for doing that.

@wkerzendorf Before making a Flags class, let's finish the error class implementation first as I am forseeing some problems with how it works (more on that elsewhere), and the problems will probably be the same for a Flags class.

sure!

one more thing unrelated to the above discussion: Can we change the name of the boolmask property to valid? (Or something more descriptive?)

how about just mask?

There's already a mask property. boolmask gives ~self.mask.

I see what you mean. I mean do we really need something like this? you could always do: ~myfancydata.mask.
I mean if lot's of people want that then yes, but otherwise...

Agreed, I would also be fine with removing it entirely.

I think boolmask used to make sense when the mask could be more complicated. Now it's just confusing because mask is a boolean mask, and boolmask=~mask - how confusing! I'll remove it :-)

Also, I agree with @kbarbary that we should finish the error class implementation before worrying about flags.

yep, I changed my error implementation a bit and pushed it up, check out PR #223

Thanks guys, I'm just catching up on this thread, but I don't think I have much to add as this point. I've usually been of the opinion that things should remind as flexible as possible and that looks like the case that has ended up here. I definitely agree that masks should be the same shape as the array as I think it will make its behavior more transparent to the end user. So basically, I think I would have been arguing for the same conclusions you came to here.

@eteq - any objections to merging?

This looks fine to me as it stands... I'm glad @kbarbary managed to channel me on the maskedarray performance bit :) - I was about to say that when he mentioned it.

I am concerned, as @astrofrog is, with the potential extra confusion of allowing broadcastable flags. flexibility is not always good, if that flexibility gives you enough range-of-motion to hang yourself... But I would say at this point we can give it a try. Practically speaking it probably won't matter much - if after a few tries at using it, we realize it's gotten confusing, we can revert back to shape-match-only in v1.0 .

I'll go ahead and merge this in another day or so if there are no further comments.