Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed Pearson r to handle masked data in a more reasonable way #1540

Closed

Conversation

niallrobinson
Copy link
Contributor

Peaerson r now only considers a subset of the data where corresponding data in both cubes are not masked. This has implications for the assumed length of the cubes, and can change the calculated r values

in response to #1534

@@ -30,7 +30,8 @@ def _get_calc_view(cube_a, cube_b, corr_coords):
"""
This function takes two cubes and returns cubes which are
flattened so that efficient comparisons can be performed
between the two.
between the two. If the arrays are maksed then only values
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a typo *masked

@ajdawson
Copy link
Member

I found the logic a bit hard to follow on first read. I'd personally prefer something a bit more concise, I think the following is equivalent to lines 101-120:

a_mask = ma.is_masked(reshaped_a) and reshaped_a.mask
b_mask = ma.is_masked(reshaped_b) and reshaped_b.mask
mask_union = a_mask | b_mask
if not np.isscalar(mask_union):
    return_a = reshaped_a.compress(~mask_union)
    return_b = reshaped_b.compress(~mask_union)
else:
    return_a = reshaped_a
    return_b = reshaped_b
return return_a, return_b, res_ind

I guess there could be arguments against this too, but at least there are fewer lines of code to understand.

@ajdawson
Copy link
Member

ajdawson commented Jul 7, 2015

@niallrobinson - This work would be superseded by the changes in #1714. The changes there make handling masks and weights a more manageable task.

@ajdawson
Copy link
Member

I'm closing this since the whole pearsonr code has subsequently been re-written.

@ajdawson ajdawson closed this Sep 22, 2015
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