Skip to content

Conversation

DocOtak
Copy link
Contributor

@DocOtak DocOtak commented Jun 25, 2022

This PR replaces the match_args_return decorator in _wrapped_ufuncs with the one proposed/discussed in #97 (comment) .

I don't think it's ready for merging: needs tests, more discussion, and the non ufunc functions need to be considered. But I wanted to show what a change like this might look like.

@efiring
Copy link
Member

efiring commented Oct 8, 2022

This certainly looks more elegant than what I originally came up with. I'm entirely open
to making the switch once it's clear it won't break anything.

@ocefpaf
Copy link
Member

ocefpaf commented Oct 10, 2022

@DocOtak and @efiring I believe that we should mint a new release with the latest changes and fixes. Should we wait to include this PR in it or should we release it now and mint a fresh one when this is done?

@efiring
Copy link
Member

efiring commented Oct 10, 2022

I think it makes sense to release both GSW-C and GSW-Python now.

@DocOtak
Copy link
Contributor Author

DocOtak commented Oct 10, 2022

I agree, don't wait on this PR.

@ocefpaf
Copy link
Member

ocefpaf commented Oct 11, 2022

I think it makes sense to release both GSW-C and GSW-Python now.

Done and done!

@DocOtak
Copy link
Contributor Author

DocOtak commented Oct 13, 2022

I am interested in continuing work on this, but would appreciate some guidance. This decorator does not work with the non ufunc functions we have.

Reading through my code comments, I think I should at least make it clear that the where=True ufunc default is the default of all ufuncs by numpy definition, not just here in this decorator. i.e. if you don't pass a where arg, numpy passes True for you.

@DocOtak DocOtak force-pushed the masked_array_ufunc branch from 257befb to 03c29b0 Compare August 22, 2025 00:25
@efiring
Copy link
Member

efiring commented Aug 22, 2025

I'm inclined to merge this. Are there any dangers? Is there any real problem with the need to keep the old decorator for the non-ufuncs, at least initially?

@DocOtak
Copy link
Contributor Author

DocOtak commented Aug 22, 2025

A danger I can think of is the function signature change with the **kwargs being added, this being "greedy", but it does just do pass through to the wrapped ufunc, which should be safe, but I don't currently have any good ideas about what tests would look like.

This would allow any of the ufunc optional keyword arguments to be passed through, so in some ways, might be a bit nicer/cleaner wrapping of the ufuncs.

I don't see any places where masked array support is tested at all. Maybe there could be a parallel set of ufunc tests that have random (fixed seed?) masks applied and then checking that the output has the expected ORed together mask?

My updates yesterday were just trying to get it rebased so it was in a merge-able state, since I was in the code anyway. Due to changes in the wrapped ufunc generators, was more tricky than I thought it would be.

@efiring
Copy link
Member

efiring commented Aug 22, 2025

Regarding explicit masked array testing: I don't think we need to put in random masked points. Most of the testing is done with the test data and results from the Matlab version, which already has some NaNs. It should be sufficient to use "masked_invalid" to generate masked array inputs, which should then yield the same results as the original after converting the results back with "ma.filled()".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants