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

uniop_bool_helper and friends: Array support #1224

Merged
merged 6 commits into from
Sep 6, 2022
Merged

Conversation

cbm755
Copy link
Collaborator

@cbm755 cbm755 commented Sep 2, 2022

This routine is supposed to return a logical Octave array. Do linear
indexing on the flattened tolist output which will work on both Array
and Matrix. Fixes #1221.

This routine is supposed to return a logical Octave array.  Do linear
indexing on the flattened tolist output which will work on both Array
and Matrix.  Fixes #1221.
@cbm755 cbm755 changed the title uniop_bool_helper: Array support in "bool" case uniop_bool_helper and friends: Array support Sep 4, 2022
Base automatically changed from 2dloop to Array_not_Matrix September 4, 2022 23:02
Copy link
Collaborator

@alexvong243f alexvong243f left a comment

Choose a reason for hiding this comment

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

It should be all good otherwise. The test case

t = sym(true)
w = [t t]
isequal(t, t)

now works with or without Pythonic.

As a side note, I didn't know about functions such as transpose, flatten or unflatten before. Well, I should have just used transpose instead of reinventing the wheel in 136184d

Comment on lines 134 to 135
'if x is not None and x.is_Matrix:'
' r = [a for a in x.T]' % note transpose
'if isinstance(x, (MatrixBase, NDimArray)):'
' r = [a for a in flatten(transpose(x).tolist())]' % note tranpose
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should do flatten(..., levels=1) here since we know we are flattening a list of list. This could have the advantage of catching some errors in case tolist gives us something funny. WDYT?

BTW I think using flatten from https://docs.sympy.org/latest/modules/utilities/iterables.html is a good idea. I should change my code to use this as well instead of itertools.chain.from_iterable, which has a longish name and returns an iterator.

For the record, flatten is documented in https://docs.sympy.org/latest/modules/utilities/iterables.html#sympy.utilities.iterables.flatten

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, I had never read that. I agree limiting to levels=1 to avoid unsuspected surprises sounds good. I will try it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's new to me as well :)

I think the functions defined in sympy.utilities.* are really well-written and useful. I'll try to use them when applicable.

inst/@sym/logical.m Outdated Show resolved Hide resolved
inst/@sym/private/uniop_bool_helper.m Outdated Show resolved Hide resolved
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.

2 participants