Opened 8 years ago
Closed 8 years ago
#14350 closed enhancement (fixed)
Implement multiplicative inverses of cycle index series
Reported by: | agd | Owned by: | agd |
---|---|---|---|
Priority: | major | Milestone: | sage-5.11 |
Component: | combinatorics | Keywords: | species, cycle index |
Cc: | sage-combinat, MartinRubey | Merged in: | sage-5.11.beta1 |
Authors: | Andrew Gainer-Dewar | Reviewers: | Frédéric Chapoton |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
In the context of virtual species (see #14348), it is also possible to define multiplicative inverses of many species. This patch incorporates code to compute the cycle indices of these multiplicative inverse species and a div method for cycle index series.
Attachments (3)
Change History (20)
comment:1 follow-up: ↓ 2 Changed 8 years ago by
comment:2 in reply to: ↑ 1 Changed 8 years ago by
Replying to agd:
If someone with permissions to do so could delete the ".path" file I accidentally attached, that would be swell.
Done.
comment:3 Changed 8 years ago by
You probably want to call your method invert (see http://docs.python.org/2/reference/datamodel.html). And then it's likely that div's default implementation will do exactly what you want.
Other than this, you might want to ask MartinRubey? to review this patch!
Cheers,
Nicolas
comment:4 Changed 8 years ago by
Nicolas,
invert looks like it should be exactly what's needed here, but it doesn't seem to work; Sage doesn't seem to know to try to use it for div. My guess is that there's some additional step I'd need to take to tell Sage that CycleIndexSeriesRing? now (usually) has division, but I'm not sure what that is. (For this purpose, is it likely to be a problem that not every element has a multiplicative inverse?)
Thanks again for your help!
--Andrew
comment:5 Changed 8 years ago by
I've renamed the cycle index multiplicative inversion method as invert as per Nicolas' suggestion. However, I haven't been able to find a way to convince Sage to use this method on its own, so the patch still includes a thin _div_ method as well.
(Also, once again, I've made a mess of the attachments. Could the next person through with privileges to do so please remove --.2.patch? Thanks!)
comment:6 Changed 8 years ago by
- Cc MartinRubey added
comment:7 follow-up: ↓ 8 Changed 8 years ago by
Per Nicolas' suggestion, I've CC'ed Martin Rubey on this ticket. (If this is not the correct way to bring it to your attention, I am very sorry! I'm still very new to this.)
comment:8 in reply to: ↑ 7 Changed 8 years ago by
Replying to agd:
Per Nicolas' suggestion, I've CC'ed Martin Rubey on this ticket. (If this is not the correct way to bring it to your attention, I am very sorry! I'm still very new to this.)
That usually works. I am not sure what his login is though; you might want to make a quick search (in principle the list of login's in on trac's main web page, but I haven't found him there).
A private e-mail can increase the chances of success :-) In this case you probably want to send an e-mail on sage-combinat-devel for advertising your work.
Cheers,
Nicolas
comment:9 Changed 8 years ago by
I'll take a look at this.
comment:10 Changed 8 years ago by
- Status changed from new to needs_review
Per discussions elsewhere, I've uploaded a new version of the patch which was generated using "hg export tip". The content is the same.
comment:11 follow-up: ↓ 12 Changed 8 years ago by
- Description modified (diff)
for the bot: apply trac_14350_cycle_index_mult_inverses.patch
You should use python3 syntax for raise:
raise ValueError("constant term must be non-zero")
you could avoid to define parent by using directly self.parent()
you should doctest the _div_ function
comment:12 in reply to: ↑ 11 Changed 8 years ago by
Replying to chapoton:
You should use python3 syntax for raise:
raise ValueError("constant term must be non-zero")you could avoid to define parent by using directly self.parent()
you should doctest the _div_ function
I have uploaded a new patch which makes these changes. Thanks for the review!
Changed 8 years ago by
comment:13 Changed 8 years ago by
for the bot: apply trac_14350_cycle_index_mult_inverses.patch
comment:14 Changed 8 years ago by
ok, looks good to me, positive review.
comment:15 Changed 8 years ago by
- Reviewers set to Frédéric Chapoton
- Status changed from needs_review to positive_review
comment:16 Changed 8 years ago by
- Milestone changed from sage-5.10 to sage-5.11
comment:17 Changed 8 years ago by
- Merged in set to sage-5.11.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
If someone with permissions to do so could delete the ".path" file I accidentally attached, that would be swell.