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

Refactored ArrayComprehension #16929

Merged
merged 7 commits into from Jun 3, 2019

Conversation

Projects
None yet
6 participants
@czgdp1807
Copy link
Member

commented May 30, 2019

References to other Issues or PRs

[1] #16845

Brief description of what is fixed or changed

ArrayComprehension, in sympy.tensor.array.array_comprehension have been refactored on the following grounds,

  1. Object creation was resulting in too many unnecessary attributes taking more space in the memory.
  2. All the computations were done at the time of object creation which is not required and makes it difficult to read the code because, all of the logic lies in __new__.
  3. Suggestion in #16845 (comment) are included in docstrings.

Other comments

The logic of the code hasn't been changed, just some shifts have been done to enhance readability and performance.

Release Notes

NO ENTRY

@sympy-bot

This comment has been minimized.

Copy link

commented May 30, 2019

Hi, I am the SymPy bot (v147). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

  • No release notes entry will be added for this pull request.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->
[1] https://github.com/sympy/sympy/pull/16845

#### Brief description of what is fixed or changed
`ArrayComprehension`, in `sympy.tensor.array.array_comprehension` have been refactored on the following grounds,
1. Object creation was resulting in too many unnecessary attributes taking more space in the memory. 
2. All the computations were done at the time of object creation which is not required and makes it difficult to read the code because, all of the logic lies in `__new__`.
3. Suggestion in https://github.com/sympy/sympy/pull/16845#discussion_r287692310 are included in docstrings.

#### Other comments
The logic of the code hasn't been changed, just some shifts have been done to enhance readability and performance.

#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

@codecov

This comment has been minimized.

Copy link

commented May 30, 2019

Codecov Report

Merging #16929 into master will increase coverage by 0.01%.
The diff coverage is 81.818%.

@@             Coverage Diff              @@
##            master    #16929      +/-   ##
============================================
+ Coverage   73.922%   73.932%   +0.01%     
============================================
  Files          620       620              
  Lines       160262    160255       -7     
  Branches     37606     37604       -2     
============================================
+ Hits        118469    118480      +11     
+ Misses       36305     36288      -17     
+ Partials      5488      5487       -1
@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

ping @Upabjojr

@czgdp1807 czgdp1807 changed the title [GSoC] Refactored ArrayComprehension Refactored ArrayComprehension May 31, 2019

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Generally, I prefer the code as it was before this PR. OTOH, The docstring improvements are OK.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

@Upabjojr Can you please explain how the code will slow down in each of the case?

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

You are replacing pre-computed values with loops. So O(N) instead of O(1).

czgdp1807 added some commits May 31, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

I have made the changes and optimised the code wherever necessary. A corner case was also uncovered. I think is_numeric should also be calculated while creating the object as it will be used in doit and user may use doit a lot many times, leading to O(N*number_of_times_doit_called).

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

is the name is_numeric standard in SymPy? do other modules use the same?

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

Sine, Product and Sum are closely related to ArrayComprehension, I expected them to have is_numeric attribute. However, they don't have.

>>> from sympy import Product, Sum
>>> atr = 'is_numeric'
>>> atr in dir(Product)
False
>>> atr in dir(Sum)
False

I believe, is_number would be better in such cases and is being used across various modules of SymPy.

@kangzhiq

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

is the name is_numeric standard in SymPy? do other modules use the same?

No it is not. The idea does come from is_number. I was thinking that, semantically, a product or a summation can be a number, but it is less understandable to say that a list is number. That is why I changed it to is_numeric, which indicates if the expanded list is numeric.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

Would it possible to use is_symbolic as it is also a standard in sympy?

@kangzhiq

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Would it possible to use is_symbolic as it is also a standard in sympy?

I think that's ok. Let's see what other contributors say.

@smichr

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Just out of curiosity, why aren't we just creating something like Range that can take symbolic parameters and then using ImageSet instead of a ArrayComprehension: imageset((i, j), f(i,j), Range(a, b), Range(c, d))?

Ans: because we want an ordered object, not a set.

Question: why not call it List?

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

Would it possible to use is_symbolic as it is also a standard in sympy?

It's not a symbol.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

I'm not sure about .is_numeric, but as it's already been merged, this can be handled in another PR.

Show resolved Hide resolved sympy/tensor/array/array_comprehension.py Outdated
Show resolved Hide resolved sympy/tensor/array/array_comprehension.py Outdated
Show resolved Hide resolved sympy/tensor/array/array_comprehension.py Outdated

@czgdp1807 czgdp1807 force-pushed the czgdp1807:array_comp branch from 9f7388d to af37a99 Jun 1, 2019

czgdp1807 added some commits Jun 1, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

Question: why not call it List?

I proposed this earlier, but later ArrayComprehension was agreed upon. @kangzhiq might be knowing the reason.

@smichr

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

I proposed this earlier

I would be interested to know the answer. We have Tuple, Set, Dict, OrderedDict...seems like List is filled nicely by ArrayComprehension.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

I would be interested to know the answer. We have Tuple, Set, Dict, OrderedDict...seems like List is filled nicely by ArrayComprehension.

Not really the same thing. ArrayComprehension is supposed to be a multidimensional list comprehension, not a list. It becomes a list if you call .doit() and the dimensions are non-symbolic.

@Upabjojr Upabjojr merged commit faaadab into sympy:master Jun 3, 2019

3 checks passed

codecov/project 73.932% (target 0%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sympy-bot/release-notes The release notes look OK
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.