NDArithmeticMixin and NDUncertainty Refactor #4272
Conversation
|
Short Summary
I think I forgot to mention some further changes and I hope this is not to much change for you to give me some comments. |
|
One issue I found about the I hope you understand what I mean, if not I could wrap this idea in code (just superficially). But maybe I'm thinking too much about it and it is fine just like it is. |
| function that is used by the name of the operation better than by | ||
| the function itself. Suppose we have ``data`` that is a | ||
| `~numpy.matrix` or ``Pandas.DataFrame`` we need to use different | ||
| UFUNCS (maybe) and that explodes the number of if/elif checks in |
There was a problem hiding this comment.
I think this (need to use different UFUNCS) isn't actually an issue per http://pandas.pydata.org/pandas-docs/stable/dsintro.html#dataframe-interoperability-with-numpy-functions
Even if it were an issue I think it would be better to make operation a function that defaults to the appropriate numpy ufunc because then the user can substitute a different, more appropriate, ufunc without needing to subclass or add a special case via a PR.
There was a problem hiding this comment.
Just to get this straight before I change it:
add should provide the numpy ufunc (np.add) and pass it to _arithmetic as operation? So that another ufunc could be provided by calling _arithmetic directly?
Another thing: How should I evaluate the function in _arithmetics_meta: compare the function name or simply apply the operation-parameter?
For uncertainty_propagation I'll need to compare the operation in a way, should I compare it like operation is np.add or by operation.__name__ == 'add'?
I'm not sure about what would be best, that was one another reason why I thought string comparisons were the best way to go.
There was a problem hiding this comment.
addshould provide the numpy ufunc (np.add) and pass it to_arithmeticas operation? So that another ufunc could be provided by calling_arithmeticdirectly?
Mostly, yes. I think what I said before about substituting a different function without subclassing doesn't actually work, but a subclass that wanted to use a different addition could do so by just overriding add without needing to change anything in _arithmetic.
The other advantage of making this change is that you eliminate the need for the duplicate logic in _arithmetic_data and _arithmetic_meta
There was a problem hiding this comment.
Another thing: How should I evaluate the function in
_arithmetics_meta: compare the function name or simply apply theoperation-parameter?
I would just apply the parameter.
There was a problem hiding this comment.
For uncertainty_propagation I'll need to compare the operation in a way, should I compare it like operation is
np.addor byoperation.__name__ == 'add'?
Good question...looking over the code, I'd lean towards using operation.__name__. What about an approach like that in the current implementation [1]? That adds an argument to _arithmetic that just gets passed through to the uncertainty call but that maybe isn't so bad.
|
@MSeifert04 -- lots of inline comments, I still want to try running the tests locally and see if it breaks anything in ccdproc :). Hoping to do that today. |
|
@mwcraig Thanks for the comments, I've adressed every obvious comment but left replies where I wasn't sure how to proceed. If you need any help with |
|
@jehturner -- would you mind looking over this? It would impact |
|
OK. Any sane changes to the arithmetic are unlikely to break things for me, as I'm not systematically using that functionality yet (though obviously that's the intention). Most of the calculations are done by the external processes I'm initially wrapping and then gradually replacing, something I'm just completing the outline of. I can certainly do a quick check but it might take a few days as I'm juggling 2 deadlines. Anyway, thanks for asking and thanks to Michael for improving the code. |
|
I confirm that this does not break my code (in a couple of dozen quick tests). Sorry it has taken so long to comment (has it really been 3 weeks?). I haven't yet looked closely at what these changes actually do but I'm not noticing any unexpected problems. |
|
Thanks for checking it out @jehturner --the fact that it works for you is encouraging. I hope to review these changes soon myself. |
|
@mwcraig @embray Should I incorporate @mwcraig suggestions or still wait a bit longer? @jehturner Thanks for the tests. |
|
@MSeifert04 -- hold off for a couple more days, hoping to get comments form @wkerzendorf (he was traveling for most of November). |
| @@ -23,212 +168,638 @@ class NDArithmeticMixin(object): | |||
| When subclassing, be sure to list the superclasses in the correct order | |||
There was a problem hiding this comment.
Is there a way to automate or force the correct order? @embray you know a lot of python magic.
There was a problem hiding this comment.
It is probably worth checking what makes it required to have a certain order. Often, one can write things such that order does not matter (but certainly not always).
There was a problem hiding this comment.
The MRO dictates the order - a helpful reference is http://nedbatchelder.com/blog/201210/multiple_inheritance_is_hard.html and the comments underneath.
If the mixin overrides a method in NDDataBase you want the mixin listed before the base class.
It might be better to simply point out that order matters and provide a reference....
correlation with np.ndarray now works added 2 tests regarding correlation documentation fixes
|
@jehturner - Sorry, I haven't seen your comments. I just used the @mwcraig - I rebased and deleted the provisional code parts. I hope I have time the next days to make an accurate Changelog entry. |
|
I've added a small changelog entry. I think it covers everything related to public methods. |
|
@MSeifert04 @mwcraig and all the reviewers - Lots of great work here, +:100: ! I have one concern, though, and that's the lack of (narrative) documentation and examples, particularly with the correlation stuff (which is notoriously tricky). I see that #4538 was created as a reminder to do that. But, in my experience, if something gets merged with a separate "do the documentation" issue, often the documentation doesn't get written until it's far too late (i.e., someone mis-used it or gave up on it because they didn't understand it). That said, I understand you might want to get it in soon for other changes. So @mwcraig, I'm fine with you making a command decision about whether my concern here is warranted or if you're confident you and/or @MSeifert04 will get the docs in for this in a reasonably timely fashion before the key points have escaped your heads. |
|
Oh one other thing that's more informational than anything else: I think we'll also want to try to at least get a start on something with |
|
Agreed to let |
|
@eteq -- I'm going to go ahead and merge this; @MSeifert04 has an outline in an ipython notebook in #4538. Agreed it would have been better to flesh out the docs at the same time, but this has lingered long enough I'd like to get it closed. I'll work with @MSeifert04 to divide up the work on the docs with a target date of a week or two. |
NDArithmeticMixin and NDUncertainty Refactor
Initial try of splitting the large #4234. More information will follow when the splitting is done.
edit 12/22/2015: added checklist
Next steps:
Nonewith a warning if the methods don't exist._arithmetic_meta