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

Jump to bottom

Refactor domainmatrix.py to sympy.polys.matrices package #20759

Merged
merged 6 commits into from Jan 12, 2021
Merged

Refactor domainmatrix.py to sympy.polys.matrices package #20759

merged 6 commits into from Jan 12, 2021

Conversation

oscarbenjamin
Copy link
Contributor

@oscarbenjamin oscarbenjamin commented Jan 7, 2021 •
edited

References to other Issues or PRs

Previous work on this is in #20621, #19882, #18844

Brief description of what is fixed or changed

Refactor domainmatrix.py by making a sympy.polys.matrices package and splitting it up into several files. This code is expected to grow and will need to be better organised.

Other comments

Release Notes

  • polys
    • The DomainMatrix class has moved from sympy.polys.domainmatrix to sympy.polys.matrices and should now be imported as from sympy.polys.matrices import DomainMatrix.

@oscarbenjamin
maint(polys): add sympy.polys.matrices package
fa98667 
Refactor domainmatrix.py into a package sympy.polys.matrices
@oscarbenjamin
doc(polys): add module docstrings in sympy.polys.matrices
4c2bc06 
Add basic docstrings for each of the modules in sympy.polys.matrices
just to give some explanation of what each module is for and the classes
that they define.
@oscarbenjamin
docs(polys): add docstring to redundant domainmatrix.py
da196f0
@sympy-bot
Copy link

sympy-bot commented Jan 7, 2021 •
edited

✅

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

  • polys
    • The DomainMatrix class has moved from sympy.polys.domainmatrix to sympy.polys.matrices and should now be imported as from sympy.polys.matrices import DomainMatrix. (#20759 by @oscarbenjamin)

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

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://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->

Previous work on this is in #20621, #19882, #18844

#### Brief description of what is fixed or changed

Refactor domainmatrix.py by making a sympy.polys.matrices package and splitting it up into several files. This code is expected to grow and will need to be better organised.

#### 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 -->
* polys
    * The DomainMatrix class has moved from `sympy.polys.domainmatrix` to `sympy.polys.matrices` and should now be imported as `from sympy.polys.matrices import DomainMatrix`.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@sympy-bot
Copy link

sympy-bot commented Jan 7, 2021 •
edited

🟠

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

  • fa98667:
    • sympy/polys/matrices/__init__.py
    • sympy/polys/matrices/ddm.py
    • sympy/polys/matrices/dense.py
    • sympy/polys/matrices/domainmatrix.py
    • sympy/polys/matrices/eigen.py
    • sympy/polys/matrices/exceptions.py
    • sympy/polys/matrices/tests/__init__.py
    • sympy/polys/matrices/tests/test_ddm.py
    • sympy/polys/matrices/tests/test_dense.py
    • sympy/polys/matrices/tests/test_domainmatrix.py

The following commits delete files:

  • fa98667:
    • sympy/polys/tests/test_domainmatrix.py

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

@czgdp1807 czgdp1807 added the polys label Jan 8, 2021
@oscarbenjamin
Copy link
Contributor Author

@sylee957 does this look okay to you?

I'm building on this to add sdm.py for sparse matrices (for a separate PR).

jksuom
jksuom reviewed Jan 8, 2021
View reviewed changes
sympy/polys/matrices/__init__.py Outdated

The main export from this package is the DomainMatrix class which is a
lower-level implementation of matrices based on the polys Domains. This
implentation is typically a lot faster than sympy's standard Matrix class but
Copy link
Member

Choose a reason for hiding this comment

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

implementation

jksuom
jksuom reviewed Jan 9, 2021
View reviewed changes
sympy/polys/matrices/ddm.py
return not self.__eq__(other)

@classmethod
def zeros(cls, shape, domain):
Copy link
Member

Choose a reason for hiding this comment

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

How about making this an instance method. That might suffice for internal use in most cases. There could also be a class method with another name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do. I wouldn't want to do that in this PR because it's just for the refactoring so any nontrivial changes should be separate.

Do you mean that if it was an instance method then it could use the shape/domain from the instance?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. That is probably what we would usually want.

@oscarbenjamin oscarbenjamin mentioned this pull request Jan 12, 2021
Merged
@oscarbenjamin
Copy link
Contributor Author

Can this be merged?

All this does is move the code around to reorganise things ready for future expansion. There is plenty of work to be done on this code but I don't want to mix up the changes that just move the files around with the actual code changes.

@sylee957 sylee957 merged commit e79d97c into sympy:master Jan 12, 2021
@oscarbenjamin
Copy link
Contributor Author

Thanks @sylee957!

@sayandip18 sayandip18 mentioned this pull request Feb 6, 2021
Open
@oscarbenjamin oscarbenjamin deleted the pr_dm_refactor branch February 19, 2021 01:02
@oscarbenjamin oscarbenjamin mentioned this pull request Feb 20, 2021
Open
18 tasks
@oscarbenjamin oscarbenjamin mentioned this pull request May 22, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jksuom jksuom jksuom left review comments

Assignees
No one assigned
Labels
domainmatrix polys
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@oscarbenjamin @sympy-bot @jksuom @sylee957 @czgdp1807