-
Notifications
You must be signed in to change notification settings - Fork 230
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 part
for graded objects and other improvements
#3522
base: development
Are you sure you want to change the base?
Conversation
be82d3e
to
b1eb629
Compare
f := truncate(degs, dd^C_lo, opts); | ||
complex hashTable for i from lo+1 to hi list i => ( | ||
f = inducedTruncationMap(source f, truncate(degs, C_i, opts), dd^C_i)) | ||
)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid calling Truncation
within part
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not call truncate within part. We do however call basis.
complex hashTable for i from lo+1 to hi list i => ( | ||
f = inducedBasisMap(source f, image basis(deg, C_i, opts), dd^C_i)) | ||
)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The phrase basis
of complex seems particularly unfortunate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my perspective, the proposed changes need (at a minimum) documentation and tests before being accepted. In particular, I suspect that several of the methods do not typically return well-defined complexes or complex maps.
Tests have been included, I'm happy to add some documentation this week if otherwise the changes are acceptable.
I'm happy to remove Alternatively, would it make sense to have an internal method for |
I removed cover, super, ambient, and changed part to use an unexported |
I also changed this PR to only depend on 4 specific commits from the basis PR, which in particular don't include any breaking change. |
This PR is on top of #3519.
Summary of changes:
part(ZZ, Complex)
, but for other graded objectspart(ZZ, Complex)
, but the base ring is the original ring, not its coefficient ringcover
here was so I could make this simplificationThe rest are optimizations in line with #3352 to eliminate unnecessary gb computations, though there's more work left.