Skip to content

Some feedback (DO NOT MERGE) - #48

Open
fingolfin wants to merge 1 commit into
gap-packages:masterfrom
fingolfin:mh/feedback
Open

Some feedback (DO NOT MERGE)#48
fingolfin wants to merge 1 commit into
gap-packages:masterfrom
fingolfin:mh/feedback

Conversation

@fingolfin

Copy link
Copy Markdown
Member

No description provided.

Comment thread gap/findPQuotients.gi
IH := Image(Iso);

# Create the Isomorphism to the group structure of the `p`-Module `M`
# TODO: have you considered using anupq here instead of PQuotient?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No I haven't considered it. Didn't know it existed. Thanks for the hint :)

Comment thread gap/findPQuotients.gi
#############################################################################

BindGlobal("LINS_OGL", function(s, p)
BindGlobal("LINS_OGL", function(s, p) # TODO: could use GAP function SizeGL instead (it is not listed in the reference manual; maybe it should be...)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good to know

Comment thread gap/findPQuotients.gi
od;

# maximal integer $s$ such that $[G : H] * p ^ s <= n$
# TODO: isn't this just `s := LogInt(n/index, p);` ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, but my brain stopped working at this line of code 🤦

Comment thread gap/findPQuotients.gi
break;
fi;
# TODO: note that group Q will have a non-trivial p-quotient
# iff 0 or p is in AbelianInvariants(Q)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is not quite right, as also a power of p might be in the AbelianInvariants`.

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