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

DOC: Partially update compress_{cols,rows} #22287

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

pojaghi
Copy link

@pojaghi pojaghi commented Sep 16, 2022

Partially addresses #22269 as part of #GHC22

UCSC added 2 commits Sep 16, 2022
issue-22269 compress_cols

issue-22269 compress_cols

issue-22269 compress_cols,compress_rows
@HaoZeke HaoZeke changed the title My334sol DOC: Partially update compress_{cols,rows} Sep 16, 2022
>>> np.ma.compress_nd(x)
array([[7, 8]])
Copy link
Contributor

@rossbar rossbar Sep 16, 2022

Choose a reason for hiding this comment

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

There's something wrong here: the input array doesn't contain the values 7, 8.

Copy link
Author

@pojaghi pojaghi Sep 17, 2022

Choose a reason for hiding this comment

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

Sorry, The x should be x = np.ma.array(np.arange(9).reshape(3, 3), mask=[[1, 0, 0],[1,0,0],[0,0,0]]. I will fix it now.

[1,0,0],
[0,0,0]])
Copy link
Member

@seberg seberg Sep 19, 2022

Choose a reason for hiding this comment

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

Suggested change
[1,0,0],
[0,0,0]])
... [1, 0, 0],
... [0, 0, 0]])

Not sure how we format the new line, but this should be OK. You do need the ... to continue the input line though, otherwise it looks like a single line with output (and the test fails).

Examples
--------
>>> x = np.ma.array([1, 3, 4, 6], mask=[0, 0, 0, 1])
Copy link
Member

@seberg seberg Sep 19, 2022

Choose a reason for hiding this comment

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

Suggested change
>>> x = np.ma.array([1, 3, 4, 6], mask=[0, 0, 0, 1])
>>> x = np.ma.array([1, 3, 4, 6], mask=[0, 0, 0, 1])

There should only be a single space here, this also trips the test suite. (Also below)

>>> np.putmask(x, x>2, x**2)
>>> x
array([[ 0, 1, 2],
[ 9, 16, 25]])
Copy link
Member

@seberg seberg Sep 19, 2022

Choose a reason for hiding this comment

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

I suspect this should align properly:

Suggested change
[ 9, 16, 25]])
[ 9, 16, 25]])

Should do it (I think). Although it probably doesn't trip the test suite at least.

@seberg
Copy link
Member

seberg commented Sep 19, 2022

Thanks, I only had a brief look, but it does look like a nice, comprehensive addition of docs. There were also a few places with a two blank lines instead of one.
Mainly it would be good to make sure the test suite passes, it is this one:

azure-pipeline numpy.numpy (ComprehensiveTests macOS Python38)

which checks that the code you put actually runs correctly.

@InessaPawson
Copy link
Member

InessaPawson commented Sep 29, 2022

@pojaghi Please let me know if you need any help in implementing the suggestions from @seberg (see above).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Pending authors' response
Development

Successfully merging this pull request may close these issues.

None yet

4 participants