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

Made LinearOperator independent of spLinearOperator #482

Merged
merged 11 commits into from
Feb 19, 2023

Conversation

rohanbabbar04
Copy link
Contributor

For issue #328

  • Added _AdjointLinearOperator, _TransposeLinearOperator and _ProductLinearOperator
  • Added methods __matmul__,__rmatmul__ , transpose and adjoint
  • Made all classes extend the LinearOperator

Still need to change/update the docs, if code seems fine...

@mrava87
Copy link
Collaborator

mrava87 commented Feb 4, 2023

Thank you!

I'll try to review this in coming days and let you know :)

This commit cleans up the linearoperator module, reducing
the use of LinearOperator and aslinearoperator only to
those cases where it is stricly needed - mostly to allow mix
pylops and scipy operators as much as possible.
Copy link
Collaborator

@mrava87 mrava87 left a comment

Choose a reason for hiding this comment

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

Here are some comments. I have also made a commit where I propose changes according to my comments.

@rohanbabbar04, feel free to go over and let me know what you think.

pylops/linearoperator.py Outdated Show resolved Hide resolved
pylops/linearoperator.py Show resolved Hide resolved
pylops/linearoperator.py Show resolved Hide resolved
pylops/linearoperator.py Show resolved Hide resolved
pylops/linearoperator.py Show resolved Hide resolved
pylops/linearoperator.py Show resolved Hide resolved
@rohanbabbar04
Copy link
Contributor Author

Thank You @mrava87 for reviewing my PR, I have updated the docs and agree with all your changes.
Do take a look and give your opinion

@mrava87
Copy link
Collaborator

mrava87 commented Feb 10, 2023

Thank You @mrava87 for reviewing my PR, I have updated the docs and agree with all your changes.
Do take a look and give your opinion

Will do soon :)

@mrava87
Copy link
Collaborator

mrava87 commented Feb 11, 2023

I just identified a problem with describe due to some of the internal linop methods being moved from scipy to native pylops ones...

For example:

A = pylops.MatrixMult(np.ones((10, 5)))
A.name = "A"
C = pylops.MatrixMult(np.ones((10, 5)))

# Sum
D = A + C
describe(D)

now returns A where: {'A': '_SumLinearOperator'} instead of A+M where: {'A': 'MatrixMult', 'M': 'MatrixMult'}.

Currently investigating why...

@rohanbabbar04
Copy link
Contributor Author

Is the bug fixed?

@rohanbabbar04
Copy link
Contributor Author

Just tested the code
Now the describe works fine

A + M
where: {'A': 'MatrixMult', 'M': 'MatrixMult'}

@rohanbabbar04
Copy link
Contributor Author

Just tested the code Now the describe works fine

A + M
where: {'A': 'MatrixMult', 'M': 'MatrixMult'}

So I am removing the docstrings to make the Codacy Static Code Analysis successful
Removing this and making a commit

@mrava87
Copy link
Collaborator

mrava87 commented Feb 11, 2023

Yes the code is fixed :)

@rohanbabbar04
Copy link
Contributor Author

rohanbabbar04 commented Feb 11, 2023

Yes the code is fixed :)

@mrava87 Made the commit all checks are now successful...

Copy link
Collaborator

@mrava87 mrava87 left a comment

Choose a reason for hiding this comment

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

I made some extra changes as you completely removed any mention to scipy LinearOperator, which instead is worth still mentioning (also because we can still mix our operators with theirs - and with their solvers!)

@mrava87
Copy link
Collaborator

mrava87 commented Feb 11, 2023

@rohanbabbar04 I am satisfied with the current status of the PR.

Since we are making changes to a core method of PyLops, I want to get the feedback of another core developer before merging.. please be patient, we will let you know soon when this is ready to go :)

@cako
Copy link
Collaborator

cako commented Feb 17, 2023

I'll be trying this patch very soon! Sorry for the delays!

Copy link
Collaborator

@cako cako left a comment

Choose a reason for hiding this comment

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

This is great stuff. Looks good to me!!

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.

None yet

3 participants