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

twisted modules #2807

Merged
merged 3 commits into from
Sep 18, 2023
Merged

twisted modules #2807

merged 3 commits into from
Sep 18, 2023

Conversation

wdecker
Copy link
Collaborator

@wdecker wdecker commented Sep 16, 2023

No description provided.

@codecov
Copy link

codecov bot commented Sep 16, 2023

Codecov Report

Merging #2807 (afe21d3) into master (d0e775e) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           Coverage Diff           @@
##           master    #2807   +/-   ##
=======================================
  Coverage   73.63%   73.63%           
=======================================
  Files         455      455           
  Lines       64536    64566   +30     
=======================================
+ Hits        47520    47543   +23     
- Misses      17016    17023    +7     
Files Changed Coverage
src/Modules/ModulesGraded.jl 80.00%


function twist(M::SubquoModule{T}, g::GrpAbFinGenElem) where {T<:MPolyDecRingElem}
R = base_ring(M)
@req parent(g) == grading_group(R) "Group element not contained in grading group of base ring"
Copy link
Member

Choose a reason for hiding this comment

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

Is == here enough or do we need ===? (same at other places)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fieker Please clearify!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sideremark:
If === is needed, this problem is not restricted to this PR. The problem then also occurs in Modules/ModulesGraded.jl (and possibly other files) as the same kind of comparison also occurs there with ==.

Copy link
Member

Choose a reason for hiding this comment

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

I think == and === are the same for rings, so either is fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As confirmed by @fieker, it is as I thought: There is no difference between == and === here. That is, the PR can be merged and no other action is needed.

wdecker and others added 2 commits September 17, 2023 12:16
@jankoboehm
Copy link
Contributor

Thanks for spotting, it should be ===, and it should also be changed in set_grading!, graded_free_module, and grade, every time the same copied line.

```
"""
function twist(M::ModuleFP{T}, g::GrpAbFinGenElem) where {T<:MPolyDecRingElem}
error("Not implemented for the given type")
Copy link
Member

Choose a reason for hiding this comment

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

I am not going to block this PR over this, but, normally we strive to indent code with (at least) two spaces.

(But of course we are already widely inconsistent about this, which is why I am not asking for it to be changed here and now, just wanted to point it out)

@afkafkafk13 afkafkafk13 merged commit f5295f0 into master Sep 18, 2023
13 of 15 checks passed
@afkafkafk13 afkafkafk13 deleted the Wolfram branch September 18, 2023 11:41
fieker pushed a commit that referenced this pull request Sep 29, 2023
* twisted modules
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.

5 participants