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

Add functionality for infinity norm of matrices #13986

Merged
merged 1 commit into from
Jan 22, 2018

Conversation

RavicharanN
Copy link
Contributor

@RavicharanN RavicharanN commented Jan 21, 2018

References to other Issues or PRs

Fixes #13985.

Brief description of what is fixed or changed

Functionality for the infinity norm (maximum of absolute row sums) has been added.
DocString has been modified and also the corresponding test has been added.

Copy link
Contributor

@normalhuman normalhuman left a comment

Choose a reason for hiding this comment

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

The docstring of norm method should be edited to reflect the existence of inf-norm.

A test needs to be added too.

The proposed addition of "-inf norm" needs some justification: what other CAS implement this norm, and how? Unlike positive-order norms, the negative orders lack universally accepted mathematical interpretation.

Also, please edit the PR text according to Wiki page Writing pull request title and description

m = self.applyfunc(abs)
return Max(*[sum(m.row(i)) for i in range(m.rows)])

elif ord == S.NegativeInfinity: # Negative Infinity Norm - Maximum column sum
Copy link
Contributor

Choose a reason for hiding this comment

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

The "negative infinity norm" or matrices would not be maximum column sum. The maximum column sum is 1-norm, already implemented above.

If we want to introduce -oo norm for matrices, that merits a separate discussion. For example, Matlab docs do not define it.

Generally, one may think that the order -p norm for matrices should be min||Ax||_p over ||x||_p = 1 (this is how it works for p=2 currently) but this is just one interpretation.

@RavicharanN
Copy link
Contributor Author

Sure, I'll make the changes

@RavicharanN
Copy link
Contributor Author

@normalhuman Can you please review it now?

@normalhuman
Copy link
Contributor

The table of norms in the docstring of the method should have an entry for oo norm: see the line 3381.

@RavicharanN
Copy link
Contributor Author

PR updated

Copy link
Contributor

@normalhuman normalhuman left a comment

Choose a reason for hiding this comment

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

Looks good.

In the future, please follow the Developer Workflow, in particular: (1) create a branch for each feature (instead of committing to master), (2) follow GitHub rules for automatic closing of issues by writing "Fixes #NNNNN" or "Closed #NNNNN" in the text of a pull request.

@normalhuman normalhuman merged commit 2b781ae into sympy:master Jan 22, 2018
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.

Implementation of infinity norm for matrices
2 participants