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

Fix bug with comparison to 1 in IndexedFreeModuleElement #34084

Closed
trevorkarn opened this issue Jun 26, 2022 · 11 comments
Closed

Fix bug with comparison to 1 in IndexedFreeModuleElement #34084

trevorkarn opened this issue Jun 26, 2022 · 11 comments

Comments

@trevorkarn
Copy link
Contributor

There is a monomial basis comparison to 1 in IndexedFreeModule element. This ticket expands it to a comparison with .one_basis().

CC: @tscrim @trevorkarn

Component: linear algebra

Keywords: gsoc2022, one basis, indexed free module

Author: Trevor K. Karn

Branch/Commit: 2269b23

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/34084

@tscrim
Copy link
Collaborator

tscrim commented Jun 27, 2022

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jun 27, 2022

comment:2

This is dangerous as a module does not have to implement a one_basis() (in fact, some of them do not because it is not an element of the basis). I agree with the principle of this change, but you should pull out getting the self.parent().one_basis() outside of the loops and into a try-except AttributeError: block. I would set the stored one_basis variable you'll need to None for the exception branch.

The same change should be done in _unicode_art_ as well.

@trevorkarn
Copy link
Contributor Author

comment:3

Sounds good. Is there any examples of modules that don’t implement .one_basis that come to mind? I want to make sure I can add an example in the doctest where the fix I had breaks.

@tscrim
Copy link
Collaborator

tscrim commented Jun 29, 2022

comment:4

I think one of the bases for the DescentAlgebra doesn’t have a one_basis() since one() is a sum of basis elements. There is also a basis of one of the combinatorial Hopf algebras called the orbit basis that also has this IIRC.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2022

Changed commit from c767729 to 382cb46

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

382cb46Fix to work with algebras without a one_basis

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2022

Changed commit from 382cb46 to 2269b23

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

2269b23Change one in _unicode_art_

@tscrim
Copy link
Collaborator

tscrim commented Jul 28, 2022

comment:8

Thanks. LGTM.

@tscrim
Copy link
Collaborator

tscrim commented Jul 28, 2022

Changed keywords from gsoc2022 one-basis indexed-free-module to gsoc2022, one basis, indexed free module

@vbraun
Copy link
Member

vbraun commented Aug 4, 2022

Changed branch from u/tkarn/indexed-element-one-basis to 2269b23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants