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

Added functions to ArrayComprehension #16969

Merged
merged 2 commits into from Jun 7, 2019

Conversation

Projects
None yet
5 participants
@kangzhiq
Copy link
Contributor

commented Jun 4, 2019

References to other Issues or PRs

Brief description of what is fixed or changed

  • Added .tolist() and tests
  • Added .tomatrix() and tests
  • Refactorised _expand_array()

Other comments

Release Notes

  • tensor
    • Added functions to ArrayComprehension.
Added functions to ArrayComprehension
- Added .tolist() and tests
- Added .tomatrix() and tests
- Refactorised _expand_array()
@sympy-bot

This comment has been minimized.

Copy link

commented Jun 4, 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.

Your release notes are in good order.

Here is what the release notes will look like:

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

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. -->


#### Brief description of what is fixed or changed
- Added .tolist() and tests
- Added .tomatrix() and tests
- Refactorised _expand_array()


#### Other comments


#### 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 -->
* tensor
  * Added functions to ArrayComprehension.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@codecov

This comment has been minimized.

Copy link

commented Jun 4, 2019

Codecov Report

Merging #16969 into master will increase coverage by 0.017%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##            master    #16969       +/-   ##
=============================================
+ Coverage   73.954%   73.971%   +0.017%     
=============================================
  Files          620       620               
  Lines       160403    160503      +100     
  Branches     37637     37655       +18     
=============================================
+ Hits        118625    118727      +102     
- Misses       36293     36295        +2     
+ Partials      5485      5481        -4
@@ -272,4 +273,59 @@ def _array_subs(arr, var, val):
list_gen.append(list_expr.subs(var, val))
else:
list_gen.append(_array_subs(list_expr, var, val))
return ImmutableDenseNDimArray(list_gen)
return list_gen

This comment was marked as resolved.

Copy link
@czgdp1807

czgdp1807 Jun 5, 2019

Member

Why ImmutableDenseNDimArray is shifted to doit()? Something related to performance?

This comment was marked as resolved.

Copy link
@czgdp1807

czgdp1807 Jun 5, 2019

Member

Ah! got it, it is being used in tomatrix, etc. So returning a generalized expression for type casting. No issues. Resolve this conversation.


if not self.is_numeric:
raise ValueError("A symbolic array cannot be expanded to a matrix")
if self.rank() != 2:

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jun 5, 2019

Member

I think there was something like, self._rank, can it be used here directly? As far as I remember rank also calls self._rank. A short explanation would help if the said cannot be done. Thanks.

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jun 5, 2019

Author Contributor

Yes, There is nearly no difference between self._rank and self.rank(). BUt it would be better to keep the same coding convention. So that is a mistake. Thank for pointing it out!

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jun 5, 2019

Contributor

OK

@czgdp1807

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

I think Added functions to ArrayComprehension should be replaced by Added methods to ArrayComprehension. I don't know if I am wrong conceptually.

@kangzhiq

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

be replaced by Added methods to ArrayComprehension

I am sorry but I don't get your point. Do you mean this would lead to a confusion with the function in ArrayComprehension? But I would say functions is still comprehensible as a title of PR. So I would prefer keeping it as it has been done.

>>> a.tolist()
[[11, 12, 13], [21, 22, 23], [31, 32, 33], [41, 42, 43]]
"""
if self.is_numeric:

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jun 5, 2019

Author Contributor

@Upabjojr Regarding this .is_numeric property(dicussion raised in #16929), since I am reusing it here, it would be an occasion to modify it.
I have searched over the SymPy API and it is true that this is the only case using is_numeric. So maybe it is better to follow the standard. :-)

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jun 5, 2019

Contributor

What about "is_shape_numeric"?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jun 6, 2019

Author Contributor

@Upabjojr That would be a little bit more informative than is_numeric. But it is not used in SymPy either. Anyway, I will go with is_shape_numeric if you are ok with it. :-)

>>> a.tolist()
[[11, 12, 13], [21, 22, 23], [31, 32, 33], [41, 42, 43]]
"""
if self.is_numeric:

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jun 5, 2019

Contributor

What about "is_shape_numeric"?


if not self.is_numeric:
raise ValueError("A symbolic array cannot be expanded to a matrix")
if self.rank() != 2:

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jun 5, 2019

Contributor

OK

@@ -40,3 +46,5 @@ def test_array_comprehension():
raises(ValueError, lambda: ArrayComprehension(i*j, (i, 1, 3), (j, 2, j+1)))
raises(ValueError, lambda: len(ArrayComprehension(i*j, (i, 1, 3), (j, 2, j+4))))
raises(TypeError, lambda: ArrayComprehension(i*j, (i, 0, i + 1.5), (j, 0, 2)))
raises(ValueError, lambda: b.tolist())
raises(ValueError, lambda: b.tomatrix())

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jun 5, 2019

Contributor

also check this exception on numeric shape and rank different than 2.

Renamed is_numeric and added tests
- Renamed is_numeric to is_shape_numeric
- Modified and added tests accordingly

@Upabjojr Upabjojr merged commit e727339 into sympy:master Jun 7, 2019

3 checks passed

codecov/project 73.971% (target 0%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sympy-bot/release-notes The release notes look OK
Details

@kangzhiq kangzhiq deleted the kangzhiq:Added_functions_to_AC branch Jun 8, 2019

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.