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

New file with module types #1013

Closed

Conversation

AlexD97
Copy link
Contributor

@AlexD97 AlexD97 commented Jan 25, 2022

The module types are in a new file.

Moreover, hom for free graded modules is implemented.

@fingolfin
Copy link
Member

There are a gazillion pointless merge commits in this PR, so either we squash-merge this, or the PR history is rewritten before merge to get rid of these (e.g. by @AlexD97 squashing everything into a single commit based on latest master)

@fingolfin
Copy link
Member

It is really hard to see what's going on, as you move code, and modify it, all in one go; the individual commits are no help easier.

It'd be much easier if there were two commits: one that moves all the code into the new file, but doesn't modify it otherwise; and a second which performs the actual modification. As it is, I don't think there is an effective way to review this PR, we just have to trust you

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Some minor remarks that are probably for old code, but I happened to notice it while reading...

@doc Markdown.doc"""
ModuleFP{T}

The abstract supertype of all modules. Here, all modules are finitely presented.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't an oxymoron? So perhaps:

Suggested change
The abstract supertype of all modules. Here, all modules are finitely presented.
The abstract supertype of all finitely presented modules.

ModuleFP{T}

The abstract supertype of all modules. Here, all modules are finitely presented.
The type variable `T` refers to the type of the elements of the 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.

Good, ideally all our type docstrings should explain the meaning of the various type parameters :-)

@doc Markdown.doc"""
AbstractFreeMod{T} <: ModuleFP{T}

The abstract supertype of all free modules.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The abstract supertype of all free modules.
The abstract supertype of all finitely generated free modules.

@doc Markdown.doc"""
AbstractSubQuo{T} <: ModuleFP{T}

The abstract supertype of all subquotient modules.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The abstract supertype of all subquotient modules.
The abstract supertype of all finitely presented subquotient modules.

@doc Markdown.doc"""
AbstractFreeModElem{T} <: ModuleFPElem{T}

The abstract supertype of all elements of free modules.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The abstract supertype of all elements of free modules.
The abstract supertype of all elements of finitely generated free modules.

@AlexD97 AlexD97 closed this Jan 28, 2022
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.

2 participants