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

Implement basic multivariate polynomial species #38446

Merged
merged 171 commits into from
Nov 16, 2024

Conversation

Newtech66
Copy link
Contributor

@Newtech66 Newtech66 commented Jul 29, 2024

Related to #30727.

We implement basic functionality for multivariate polynomial species, using its representation as a pair of a permutation group and a mapping between the domain of the permutation group and some variables. We provide addition, multiplication, and (partitional) composition (for some special cases). We also allow it to be constructed as a group action (or a sequence thereof). Atomic and molecular decompositions are automatically computed thanks to #38371.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

Newtech66 and others added 30 commits May 12, 2024 19:59
…groups

Also added various miscellaneous functions
Co-authored-by: Martin Rubey <axiomize@yahoo.de>
Also some fixes to _element_constructor_ ConjugacyClassesOfSubgroups
Added _repr_ for ConjugacyClassesOfSubgroups
I now output B[(gens or name if available)] as _repr_ for generators, for example B[1] + 2*B[(2,3,4)]
Comment on lines 769 to 790
def __classcall__(cls, *args, **kwds):
"""
Normalize the arguments.

EXAMPLES::

sage: from sage.rings.species import MolecularSpecies
sage: MolecularSpecies("X,Y") is MolecularSpecies(["X", "Y"])
True

sage: MolecularSpecies("X,Y") == MolecularSpecies(["X", "Z"])
False
"""
if isinstance(args[0], AtomicSpecies):
indices = args[0]
else:
assert "names" not in kwds or kwds["names"] is None
indices = AtomicSpecies(args[0])
category = Monoids().Commutative() & SetsWithGrading().Infinite()
return super().__classcall__(cls, indices,
prefix='', bracket=False,
category=category)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks actually fishy to me. I really would like to have MolecularSpecies only accept names as argument. However, if I understand correctly, MolecularSpecies.__init__ has to have the same signature, but apparently it must also have the same signature as IndexedFreeAbelianMonoid.__classcall__ (or __init__?).

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, you can explicitly skip that __classcall__ and go to its base class's one through UniqueRepresentation.__classcall__(cls, names). The __init__ and __classcall__ signatures (which can be different) only need to be able to take as input the final key/signature that is given to the UniqueRepresentation.

@mantepse
Copy link
Collaborator

mantepse commented Nov 5, 2024

I think that I adressed all other comments, please let me know if this is not the case. Most importantly: many thanks for the thorough review, I appreciate it!

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Essentially this is good to go modulo one doc detail and one potential change for you.

src/sage/rings/species.py Show resolved Hide resolved
Comment on lines 769 to 790
def __classcall__(cls, *args, **kwds):
"""
Normalize the arguments.

EXAMPLES::

sage: from sage.rings.species import MolecularSpecies
sage: MolecularSpecies("X,Y") is MolecularSpecies(["X", "Y"])
True

sage: MolecularSpecies("X,Y") == MolecularSpecies(["X", "Z"])
False
"""
if isinstance(args[0], AtomicSpecies):
indices = args[0]
else:
assert "names" not in kwds or kwds["names"] is None
indices = AtomicSpecies(args[0])
category = Monoids().Commutative() & SetsWithGrading().Infinite()
return super().__classcall__(cls, indices,
prefix='', bracket=False,
category=category)
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, you can explicitly skip that __classcall__ and go to its base class's one through UniqueRepresentation.__classcall__(cls, names). The __init__ and __classcall__ signatures (which can be different) only need to be able to take as input the final key/signature that is given to the UniqueRepresentation.

src/sage/rings/species.py Show resolved Hide resolved
@mantepse
Copy link
Collaborator

mantepse commented Nov 7, 2024

No, you can explicitly skip that __classcall__ and go to its base class's one through UniqueRepresentation.__classcall__(cls, names). The __init__ and __classcall__ signatures (which can be different) only need to be able to take as input the final key/signature that is given to the UniqueRepresentation.

Cool, that worked and looks much better! Thank you!

src/sage/rings/species.py Outdated Show resolved Hide resolved
src/sage/rings/species.py Outdated Show resolved Hide resolved
INPUT:

- ``dis`` -- a directly indecomposable permutation group
- ``domain_partition`` -- a `k`-tuple of ``frozenset``s,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- ``domain_partition`` -- a `k`-tuple of ``frozenset``s,
- ``domain_partition`` -- a `k`-tuple of ``frozenset`` entries,

This will not format properly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, should all of this information should be moved to the (public facing) class level?

mantepse and others added 2 commits November 8, 2024 02:30
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
H = _stabilizer_subgroups(S, X, a)
if len(H) > 1:
raise ValueError("action is not transitive")
return self(H[0], pi, check=check)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return self(H[0], pi, check=check)
return self._element_constructor_(H[0], pi, check=check)

This is where you expect it to always end up (without any coercion being done), right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed. I reorganized so that recursion is avoided, because it is a pain when debugging.

Also, the error message was (mathematically) wrong.

src/sage/rings/species.py Outdated Show resolved Hide resolved
src/sage/rings/species.py Outdated Show resolved Hide resolved
@mantepse
Copy link
Collaborator

mantepse commented Nov 8, 2024

Thank you for being careful, I especially like that we caught the wrong error message!

@tscrim
Copy link
Collaborator

tscrim commented Nov 8, 2024

Thank you for all the changes. If the bot comes back (morally) green, then you can set a positive review.

Copy link
Collaborator

@mantepse mantepse left a comment

Choose a reason for hiding this comment

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

Approved via tscrim

@mantepse
Copy link
Collaborator

mantepse commented Nov 8, 2024

yippie!

vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 9, 2024
    
Related to sagemath#30727.

We implement basic functionality for multivariate polynomial species,
using its representation as a pair of a permutation group and a mapping
between the domain of the permutation group and some variables. We
provide addition, multiplication, and (partitional) composition (for
some special cases). We also allow it to be constructed as a group
action (or a sequence thereof). Atomic and molecular decompositions are
automatically computed thanks to sagemath#38371.

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#38446
Reported by: Mainak Roy
Reviewer(s): Mainak Roy, Martin Rubey, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 13, 2024
    
Related to sagemath#30727.

We implement basic functionality for multivariate polynomial species,
using its representation as a pair of a permutation group and a mapping
between the domain of the permutation group and some variables. We
provide addition, multiplication, and (partitional) composition (for
some special cases). We also allow it to be constructed as a group
action (or a sequence thereof). Atomic and molecular decompositions are
automatically computed thanks to sagemath#38371.

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#38446
Reported by: Mainak Roy
Reviewer(s): Mainak Roy, Martin Rubey, Travis Scrimshaw
@vbraun vbraun merged commit 4817e52 into sagemath:develop Nov 16, 2024
19 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants