Checked if subs argument was zip object and added test #19159

gfolbe318 · 2020-04-21T06:08:33Z

matrices
- changed subs function to check if input is zip
- added tests in test_commonmatrix.py

sympy-bot · 2020-04-21T06:08:43Z

✅

Hi, I am the SymPy bot (v160). 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:

matrices
- changed subs function to check if input is zip (#19159 by @czgdp1807 and @gfolbe318)
- added tests in test_commonmatrix.py (#19159 by @czgdp1807 and @gfolbe318)

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

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.

<!-- BEGIN RELEASE NOTES -->

* matrices
    * changed subs function to check if input is zip
    * added tests in test_commonmatrix.py

<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

codecov · 2020-04-21T07:44:38Z

Codecov Report

Merging #19159 into master will increase coverage by 0.000%.
The diff coverage is 100.000%.

@@            Coverage Diff            @@
##            master    #19159   +/-   ##
=========================================
  Coverage   75.849%   75.849%           
=========================================
  Files          670       670           
  Lines       173699    173701    +2     
  Branches     40978     40979    +1     
=========================================
+ Hits        131750    131752    +2     
- Misses       36209     36213    +4     
+ Partials      5740      5736    -4

smitgajjar · 2020-04-21T11:24:15Z

Please refer this and other PR comments for writing proper PR template. Also, add a short description about what you changed and what gets fixed.

oscarbenjamin · 2020-04-21T16:47:54Z

sympy/matrices/common.py

@@ -2118,6 +2118,10 @@ def subs(self, *args, **kwargs):  # should mirror core.basic.subs
        >>> Matrix(_).subs(y, x)
        Matrix([[x]])
        """
+
+        if isinstance(args[0], zip):
+            args = (list(args[0]),)


There are other possibilities for what args[0] could be that would have this problem e.g. map etc (athough zip is most common).

I think it would be better to do something like convert args to a tuple if whenever it is not a tuple.

Also this would ignore args[1] etc if supplied.

Hey @oscarbenjamin, thanks for the feedback. In all honesty, I didn't realize that creating a fork of the repository would still notify contributors of any pull requests I made...rookie mistake! I tried inputs on maps and those seemed to work. I'll now try to enhance this pull request like you suggested and also add test cases as well as some release notes.

Also, would you recommend that I create a completely new PR? Or can I just modify this one?

Or can I just modify this one?

Modifying this one should do.

gfolbe318 · 2020-04-21T19:09:00Z

@smitgajjar I will add release notes, but I also have to modify some other files and write test cases. Should I close this PR and make another? Sorry, I am new to this.

smitgajjar · 2020-04-21T19:15:46Z

You can continue making commits on this branch (i.e. updating other files or adding tests). No need to make a new PR.

smitgajjar · 2020-04-21T19:21:00Z

Refer the Sympy's development workflow, especially patch updates here :)

sympy-bot · 2020-04-22T02:45:32Z

🟠

Hi, I am the SymPy bot (v160). I've noticed that some of your commits add or delete files. Since this is sometimes done unintentionally, I wanted to alert you about it.

This is an experimental feature of SymPy Bot. If you have any feedback on it, please comment at sympy/sympy-bot#75.

The following commits add new files:

5b643ae:
- sympy/matrices/tests/test_commonmatrix.py.23eb1923b522b32a721d23f26c57bc56.py

The following commits delete files:

d9a820f:
- sympy/matrices/tests/test_commonmatrix.py.23eb1923b522b32a721d23f26c57bc56.py

If these files were added/deleted on purpose, you can ignore this message.

gfolbe318 · 2020-04-22T03:37:02Z

I did some thought about this error and came to the conclusion that this issue might be a little more difficult than I previously expected. Here is why:

The change proposed in #19159 (comment) will not work because (from my understanding), all input types that are not iterators work perfectly. Simply casting the argument to a tuple (if it is not so already) will cause inputs that are dictionaries and sets to fail and will result in a lot of code duplication from sympy/core/basic.py. It is also worth noting that when len(args) == 2, iterators are not valid inputs to the subs function.

The original piece of code I added should fix the issue it is addressing, but I am open to keep working on it to make it more generalized. I can also try something like this:

if iterable(args[0]):
    if(isinstance(args[0], map):
         #convert args[0] to list using args[0].items
    elif isinstance(args[0], ????):
        #would this happen?
    else:
         #convert args[0] to list using casting

Essentially, how would I go about identifying that my data structure is iterable but is not a map or set without code duplication? (Sorry for the multiple edits)

oscarbenjamin · 2020-04-22T20:39:49Z

One possibility would be to do something like:

if len(args) == 1 and  not isinstance(args[0], (dict, set)) and iterable(args[0]) and not is_sequence(args[0]):
       args = (list(args[0]),)

I'm wondering though whether it might be better to punt entirely on trying to reimplement and part of subs (which is already implemented for Basic) and do:

    def subs(self, *args, **kwargs):
        return self.as_immutable().subs(*args, **kwargs).as_mutable()

czgdp1807 · 2020-05-21T06:17:35Z

@gfolbe318 Would you like to complete this PR? Please resolve the conflicts. Thanks for your contributions. Please do not close this PR.

gfolbe318 · 2020-05-21T17:43:52Z

Hello, yes. I apologize for the gaps in communication. I will take a closer look at it !

gfolbe318 · 2020-06-10T02:12:46Z

Just got back to this, apologies again for the delays. Things have been pretty hectic. I think my most recent fix should be good. I tried both of @oscarbenjamin 's suggestions and ended up going with the first one, as the second caused a recursion error. Hopefully I merged everything in correctly. Another thing I am struggling with is getting the release notes check to pass. Please let me know if you see any glaring errors with what I wrote. I have read the guidelines multiple times and looked at other examples, so I am assuming that I'm missing something small.

gfolbe318 · 2020-06-16T22:00:01Z

The Travis CI build failed, but after doing some investigation I read that it could've been due to a failed HTTP connection. How could I go about rerunning the build without committing a change?

czgdp1807 · 2020-09-05T17:27:00Z

I have resolved the conflicts. @gfolbe318 Can you see if it is okay? @oscarbenjamin Is there anything more to be done to let it in master?

oscarbenjamin · 2020-09-05T22:28:12Z

Yes, this is good. Thanks!

gfolbe318 · 2020-09-05T22:38:14Z

Thanks everyone!

made fix to matrix subs function

8d6c968

oscarbenjamin reviewed Apr 21, 2020

View reviewed changes

Abdullahjavednesar added the matrices label Apr 21, 2020

gfolbe318 changed the title ~~made fix to matrix subs function~~ Checked if subs argument was zip object and added tests Apr 22, 2020

gfolbe318 changed the title ~~Checked if subs argument was zip object and added tests~~ Checked if subs argument was zip object and added test Apr 22, 2020

added test cases

5b643ae

gfolbe318 added 2 commits April 21, 2020 22:49

deleted unnecessary file

d9a820f

removed duplicate newline

f71b1fa

czgdp1807 mentioned this pull request May 21, 2020

Checked for iterable objects in subs as well as added tests #16479

Closed

gfolbe318 added 2 commits June 9, 2020 21:14

Merge branch 'master' into subs_fix

80929fa

zip working

cfc3006

gfolbe318 and others added 2 commits June 16, 2020 18:00

Trigger

3ff4dfa

Merge branch 'master' into subs_fix

514a673

oscarbenjamin merged commit 9b40b1e into sympy:master Sep 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checked if subs argument was zip object and added test #19159

Checked if subs argument was zip object and added test #19159

gfolbe318 commented Apr 21, 2020 •

edited by czgdp1807

sympy-bot commented Apr 21, 2020 •

edited

codecov bot commented Apr 21, 2020 •

edited

smitgajjar commented Apr 21, 2020

oscarbenjamin Apr 21, 2020

gfolbe318 Apr 21, 2020

gfolbe318 Apr 21, 2020 •

edited

czgdp1807 Apr 28, 2020

gfolbe318 commented Apr 21, 2020

smitgajjar commented Apr 21, 2020

smitgajjar commented Apr 21, 2020 •

edited

sympy-bot commented Apr 22, 2020 •

edited

gfolbe318 commented Apr 22, 2020 •

edited

oscarbenjamin commented Apr 22, 2020

czgdp1807 commented May 21, 2020

gfolbe318 commented May 21, 2020

gfolbe318 commented Jun 10, 2020

gfolbe318 commented Jun 16, 2020

czgdp1807 commented Sep 5, 2020

oscarbenjamin commented Sep 5, 2020

gfolbe318 commented Sep 5, 2020

Checked if subs argument was zip object and added test #19159

Checked if subs argument was zip object and added test #19159

Conversation

gfolbe318 commented Apr 21, 2020 • edited by czgdp1807

sympy-bot commented Apr 21, 2020 • edited

codecov bot commented Apr 21, 2020 • edited

Codecov Report

smitgajjar commented Apr 21, 2020

oscarbenjamin Apr 21, 2020

Choose a reason for hiding this comment

gfolbe318 Apr 21, 2020

Choose a reason for hiding this comment

gfolbe318 Apr 21, 2020 • edited

Choose a reason for hiding this comment

czgdp1807 Apr 28, 2020

Choose a reason for hiding this comment

gfolbe318 commented Apr 21, 2020

smitgajjar commented Apr 21, 2020

smitgajjar commented Apr 21, 2020 • edited

sympy-bot commented Apr 22, 2020 • edited

🟠

gfolbe318 commented Apr 22, 2020 • edited

oscarbenjamin commented Apr 22, 2020

czgdp1807 commented May 21, 2020

gfolbe318 commented May 21, 2020

gfolbe318 commented Jun 10, 2020

gfolbe318 commented Jun 16, 2020

czgdp1807 commented Sep 5, 2020

oscarbenjamin commented Sep 5, 2020

gfolbe318 commented Sep 5, 2020

gfolbe318 commented Apr 21, 2020 •

edited by czgdp1807

sympy-bot commented Apr 21, 2020 •

edited

codecov bot commented Apr 21, 2020 •

edited

gfolbe318 Apr 21, 2020 •

edited

smitgajjar commented Apr 21, 2020 •

edited

sympy-bot commented Apr 22, 2020 •

edited

gfolbe318 commented Apr 22, 2020 •

edited