Opened 6 years ago
Closed 5 years ago
#11641 closed enhancement (fixed)
Implementation of decorator for combinatorial maps
Reported by: | stumpc5 | Owned by: | sage-combinat |
---|---|---|---|
Priority: | major | Milestone: | sage-5.7 |
Component: | combinatorics | Keywords: | combinatorial statistic |
Cc: | chrisjamesberg, saliola, zabrocki, hivert | Merged in: | sage-5.7.beta1 |
Authors: | Christian Stump | Reviewers: | Mike Zabrocki |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #13074 | Stopgaps: |
Description (last modified by )
This patch implements decorators for combinatorial maps. The aim is to be able to ask for all combinatorial maps for a given combinatorial class.
Attachments (1)
Change History (46)
comment:1 Changed 6 years ago by
- Description modified (diff)
comment:2 Changed 6 years ago by
- Description modified (diff)
comment:3 Changed 5 years ago by
- Cc chrisjamesberg saliola added; cberg@… saliola@… removed
- Description modified (diff)
- Status changed from new to needs_review
- Summary changed from Implementation of a guesser of combinatoral statistics to Implementation of decorator for combinatorial statistics and maps
- Work issues make it nice and smooth deleted
comment:4 follow-ups: ↓ 5 ↓ 9 Changed 5 years ago by
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 5 years ago by
Thanks for taking a look at the patch and the helpful suggestions. One question:
Replying to sluther:
1) Please don't use assert, raise ValueError? instead.
Why shouldn't one use assert?
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 5 years ago by
Replying to saliola:
Thanks for taking a look at the patch and the helpful suggestions. One question:
Replying to sluther:
1) Please don't use assert, raise ValueError? instead.
Why shouldn't one use assert?
- If input parameters are of the wrong type, it's usual to raise a
TypeError
. It's good to raise errors that obey conventions, because that helps writing appropriatetry ... except
clauses when needed. - assert statements can be optimized away if
python -O
is selected, so they are only suitable to assert conditions that are supposed to hold anyway (and raise an early error if they don't, to help debugging)
comment:7 in reply to: ↑ 6 ; follow-up: ↓ 8 Changed 5 years ago by
comment:8 in reply to: ↑ 7 Changed 5 years ago by
Replying to stumpc5:
@Chris, Franco: are you going to address the issues in the review or should I make the changes?
I doubt that Chris nor I will have the chance to look at this in the next couple of days, so if you get the chance to do it, then go ahead.
comment:9 in reply to: ↑ 4 Changed 5 years ago by
Replying to sluther:
I fixed the 5 issues mentioned.
These two classes share a lot of code. Have you though about a way to avoid this duplication?
I haven't really thought about this yet. If Franco / Chris have some idea, let me know...
Thanks, Christian
comment:10 Changed 5 years ago by
Fixed the bug that was occurring. Didn't have permission to delete your old patch.
comment:11 follow-up: ↓ 12 Changed 5 years ago by
Fixed some documentation and deleted blacklists from combinatorial maps (since we don't use these anymore). Accidentally added it twice and now I don't know how to delete my patch.
comment:12 in reply to: ↑ 11 ; follow-up: ↓ 17 Changed 5 years ago by
Replying to chrisjamesberg:
Fixed some documentation and deleted blacklists from combinatorial maps (since we don't use these anymore). Accidentally added it twice and now I don't know how to delete my patch.
You can't delete a patch: only accounts with admin privileges can do that.
However, you can direct the patchbot to which patches to apply.
Apply: trac_11641-combinatorial_statistics_and_maps_decorator-cs.2.patch
comment:13 Changed 5 years ago by
- Description modified (diff)
- Summary changed from Implementation of decorator for combinatorial statistics and maps to Implementation of decorator for combinatorial maps
comment:14 follow-up: ↓ 16 Changed 5 years ago by
Apply: trac_11641-combinatorial_statistics_and_maps_decorator-cs.2.patch, trac_11641-combinatorial_maps_review-fs.patch, trac_11641_concrete_combinatorial_maps_cb.patch
Hey Christian,
We made some changes. We deleted combinatorial_statistics since we will do that in another patch. For now, let's concentrate on combinatorial_maps.
Some changes:
- we changed the repr method
- we added decorators to combinatorial maps (permutations, partitions, etc.)
- we updated doctests to use the implemented combinatorial maps
But we have some questions:
- I've littered the file combinatorial_maps.py with TODOs. Take a look at them. Most are documentation issues, or things I don't understand.
- another question: why doesn't the following work?
sage: from sage.misc.combinatorial_map import combinatorial_maps_in_class sage: p = Permutation([1,3,2,4]) sage: cmaps = combinatorial_maps_in_class(p) sage: cmaps[0](p)
- should this file be in
sage.combinat.misc
orsage.combinat.combinatorial_maps
instead ofsage.misc
?
comment:15 Changed 5 years ago by
- Status changed from needs_review to needs_info
comment:16 in reply to: ↑ 14 ; follow-up: ↓ 18 Changed 5 years ago by
Replying to saliola:
Apply: trac_11641-combinatorial_statistics_and_maps_decorator-cs.2.patch, trac_11641-combinatorial_maps_review-fs.patch, trac_11641_concrete_combinatorial_maps_cb.patch
Should I delete all patches on the ticket, and you again add those we want to keep?
Hey Christian, We made some changes. We deleted combinatorial_statistics since we will do that in another patch. For now, let's concentrate on combinatorial_maps.
Alright.
Some changes: - we changed the repr method - we added decorators to combinatorial maps (permutations, partitions, etc.) - we updated doctests to use the implemented combinatorial maps
good - but I guess we are creating tons of conflicts in the queue if trac_11641_concrete_combinatorial_maps_cb.patch gets merged into main sage (though I guess we should do it anyway).
But we have some questions: - I've littered the file combinatorial_maps.py with TODOs. Take a look at them. Most are documentation issues, or things I don't understand. - another question: why doesn't the following work?
sage: from sage.misc.combinatorial_map import combinatorial_maps_in_class sage: p = Permutation([1,3,2,4]) sage: cmaps = combinatorial_maps_in_class(p) sage: cmaps[0] (p)
We want to call this function as in
sage: p.inverse()
so
sage: cmaps[0]()
returns p.whatevercmapsof0is(). Does that make sense to you?
- should this file be in
sage.combinat.misc
orsage.combinat.combinatorial_maps
instead ofsage.misc
?
Adding it to sage.combinat.misc
or sage.combinat.combinatorial_maps
sounds good to me; I basically copied the behaviour of some other decorator that happend to be in sage.misc
.
comment:17 in reply to: ↑ 12 Changed 5 years ago by
Replying to saliola:
You can't delete a patch: only accounts with admin privileges can do that.
I have admin privileges; drop me a line if you want me to delete some of the patches.
comment:18 in reply to: ↑ 16 Changed 5 years ago by
- Work issues set to move file into combinat tree ; address TODOs in the file
Replying to stumpc5:
Replying to saliola:
Apply: trac_11641-combinatorial_statistics_and_maps_decorator-cs.2.patch, trac_11641-combinatorial_maps_review-fs.patch, trac_11641_concrete_combinatorial_maps_cb.patch
Should I delete all patches on the ticket, and you again add those we want to keep?
That line that begins with "Apply:" is for the patchbot.
good - but I guess we are creating tons of conflicts in the queue if trac_11641_concrete_combinatorial_maps_cb.patch gets merged into main sage (though I guess we should do it anyway).
It has to happen eventually. We'll rebase the queue, if necessary.
But we have some questions: - I've littered the file combinatorial_maps.py with TODOs. Take a look at them. Most are documentation issues, or things I don't understand. - another question: why doesn't the following work?
sage: from sage.misc.combinatorial_map import combinatorial_maps_in_class sage: p = Permutation([1,3,2,4]) sage: cmaps = combinatorial_maps_in_class(p) sage: cmaps[0] (p)
We want to call this function as in
sage: p.inverse()so
sage: cmaps[0]()returns p.whatevercmapsof0is(). Does that make sense to you?
Using this reasoning, shouldn't the following work then? I am using Permutation_class instead of an instance of a permutation to grab the combinatorial maps:
sage: p = Permutation([1,3,2,4]) sage: Permutation_class = type(p) sage: cmaps = combinatorial_maps_in_class(Permutation_class) sage: cmaps[0](p)
Note that this also returns an error:
sage: cmaps[0]()
- should this file be in
sage.combinat.misc
orsage.combinat.combinatorial_maps
instead ofsage.misc
?Adding it to
sage.combinat.misc
orsage.combinat.combinatorial_maps
sounds good to me; I basically copied the behaviour of some other decorator that happend to be insage.misc
.
Okay, we should move it.
Did you get a chance to take a look at the TODOs in the file? There are some places that need doctests and others that need documentation, and I don't know how to do it.
comment:19 follow-up: ↓ 20 Changed 5 years ago by
Hi,
I looked at the patches and have the following preliminary comments (I haven't uploaded any changes, since it doesn't seem to be right currently):
- I deleted the files we don't need (I do have permission as well).
- trac_11641_concrete_combinatorial_maps_cb.patch is full of things we should not take care of (all the removed whitespaces) - in particular this doesn't let me apply the patch on 5.2 (and we are on holidays so I cannot download and compile 5.3.beta2).
- Examples in Franco's review do depend on trac_11641_concrete_combinatorial_maps_cb.patch, so I think the concrete maps should come right after the decorator.
- I deleted the combinatorial_statistics from my patch (and its deletion from Franco's), and then moved the decorator to sage.combinat.combinatorial_map.
- All the concrete maps I implemented in the past months are missing in the patch (e.g. the one sending area-bounce to dinv-area,and also all other maps for Dyck words).
- I looked at the TODO's in Franco's review. As I said, I simply copied another decorator, so I can't really answer all the questions (e.g. I don't know what this "f=None" is used for). If you haven't done it earlier, I will look more closely at it after next week when we are back from holidays - or if we have a rainy day in between, like today :-) .
Best, Christian
comment:20 in reply to: ↑ 19 ; follow-up: ↓ 22 Changed 5 years ago by
Hey Christian,
- I deleted the files we don't need (I do have permission as well).
Thanks!
- trac_11641_concrete_combinatorial_maps_cb.patch is full of things we should not take care of (all the removed whitespaces) - in particular this doesn't let me apply the patch on 5.2 (and we are on holidays so I cannot download and compile 5.3.beta2).
The reason for this is just that I was cutting and pasting code our old code from the combinat server. Should I really go through and put all the whitespace back in?
- Examples in Franco's review do depend on trac_11641_concrete_combinatorial_maps_cb.patch, so I think the concrete maps should come right after the decorator.
Agreed.
- I deleted the combinatorial_statistics from my patch (and its deletion from Franco's), and then moved the decorator to sage.combinat.combinatorial_map.
Perfect.
- All the concrete maps I implemented in the past months are missing in the patch (e.g. the one sending area-bounce to dinv-area,and also all other maps for Dyck words).
I didn't include all of our work from combinat because a lot of it was missing proper documentation. Our thought was, lets get this working with what's already in sage, and then adding all the maps you've created are very simple patches. I did include a few combinatorial maps, as long as the documentation was there and it was self contained, though it's possible I missed one or two.
- I looked at the TODO's in Franco's review. As I said, I simply copied another decorator, so I can't really answer all the questions (e.g. I don't know what this "f=None" is used for). If you haven't done it earlier, I will look more closely at it after next week when we are back from holidays - or if we have a rainy day in between, like today :-) .
Enjoy the holidays. Tell Anke and Jara I say hello.
comment:21 Changed 5 years ago by
- Status changed from needs_info to needs_review
- Work issues changed from move file into combinat tree ; address TODOs in the file to move file into combinat tree
comment:22 in reply to: ↑ 20 ; follow-up: ↓ 23 Changed 5 years ago by
Replying to chrisjamesberg:
The reason for this is just that I was cutting and pasting code our old code from the combinat server. Should I really go through and put all the whitespace back in?
I shrinked the patch to the stuff we want - I hope I didn't mess anything, please recheck and modify if you find anything.
Examples in Franco's review do depend on trac_11641_concrete_combinatorial_maps_cb.patch, so I think the concrete maps should come right after the decorator.
Agreed.
done.
All the concrete maps I implemented in the past months are missing in the patch (e.g. the one sending area-bounce to dinv-area,and also all other maps for Dyck words).
I didn't include all of our work from combinat because a lot of it was missing proper documentation. Our thought was, lets get this working with what's already in sage, and then adding all the maps you've created are very simple patches. I did include a few combinatorial maps, as long as the documentation was there and it was self contained, though it's possible I missed one or two.
agreed, but we have to keep in mind to prepare another trac ticket for the remaining stuff (where we shouldn't forget any of the implemented methods!).
I looked at the TODO's in Franco's review.
I went through all of them and added a review-review, please have a look!
I now wonder what the patchpot says...
Best, Christian
comment:23 in reply to: ↑ 22 Changed 5 years ago by
Replying to stumpc5:
I now wonder what the patchpot says...
Everything seems to be fine now -- would be great if one of you (Chris / Franco) could have another look and see if we should take care of anything else, or give a positive review.
I am now building 5.3.rc0. Should I then get the patches in the combinat queue or did you already do that?
Thanks, Christian
comment:24 Changed 5 years ago by
- Cc zabrocki added
Hi,
before this gets again forgotten - what is left here? I remember that I added another review patch and wanted to get your new remarks or final positive review.
I added Mike since he wants to get everything in the patch "concrete_combinatorial_maps" within dyck_words merged with stuff of him to get a medium size patch on Dyck paths done soon.
Best, Christian
comment:25 Changed 5 years ago by
- Status changed from needs_review to positive_review
comment:26 follow-up: ↓ 28 Changed 5 years ago by
Why does this have milestone sage-pending
?
comment:27 Changed 5 years ago by
- Milestone changed from sage-pending to sage-5.6
comment:28 in reply to: ↑ 26 Changed 5 years ago by
Replying to jdemeyer:
Why does this have milestone
sage-pending
?
thanks for bringing this up -- changed to 5.6...
comment:29 Changed 5 years ago by
- Reviewers set to Chris Berg
- Work issues move file into combinat tree deleted
comment:30 follow-up: ↓ 31 Changed 5 years ago by
- Status changed from positive_review to needs_work
comment:31 in reply to: ↑ 30 ; follow-up: ↓ 32 Changed 5 years ago by
Replying to stumpc5:
I will lazily import the new classes to make the pathbot (and the users waiting for their sage to start) happy. Hope, I'll be able to do it later today...
comment:32 in reply to: ↑ 31 Changed 5 years ago by
Replying to stumpc5:
Replying to stumpc5:
I will lazily import the new classes to make the pathbot (and the users waiting for their sage to start) happy. Hope, I'll be able to do it later today...
Okay, I got rid of importing types
. The function combinatorial_map
is used in Permutation
and similar others classes, and is thus imported there.
comment:33 Changed 5 years ago by
- Status changed from needs_work to needs_review
comment:34 follow-up: ↓ 35 Changed 5 years ago by
- Dependencies set to #13074
Could you rebase your patch on top of #13074 please.
comment:35 in reply to: ↑ 34 ; follow-up: ↓ 36 Changed 5 years ago by
comment:36 in reply to: ↑ 35 ; follow-up: ↓ 37 Changed 5 years ago by
Replying to stumpc5:
Chris, do you think it is worth giving this patch a positive review now?
Cheers, Christian
comment:37 in reply to: ↑ 36 ; follow-up: ↓ 38 Changed 5 years ago by
Chris, do you think it is worth giving this patch a positive review now?
I can review it too, but shouldn't it have the decorators for all of the Dyck words/permutations and other combinatorial object methods?
-Mike
comment:38 in reply to: ↑ 37 Changed 5 years ago by
Replying to zabrocki:
Chris, do you think it is worth giving this patch a positive review now?
I can review it too, but shouldn't it have the decorators for all of the Dyck words/permutations and other combinatorial object methods?
This isn't really necessary, but can as well be done in some other patch. I would actually prefer this patch to make it into Sage, so we can use the decorator in other patches; but if you want to spend the time to add the decorators to some more maps, please go ahead...
Thanks, Christian
comment:39 Changed 5 years ago by
- Status changed from needs_review to positive_review
I've looked this over now, tested it and I give it a positive review. -Mike
comment:40 Changed 5 years ago by
- Reviewers changed from Chris Berg to Mike Zabrocki
comment:41 Changed 5 years ago by
- Milestone changed from sage-5.6 to sage-5.7
comment:42 Changed 5 years ago by
- Cc hivert added
comment:43 Changed 5 years ago by
I just uploaded a minimally changed patch which requires less rebasing for the later patches in the queue (it was removing some trailing whitespace from existing code).
Christian double checked the change over my shoulder, so we leave this ticket at positive review.
Changed 5 years ago by
comment:44 Changed 5 years ago by
Rebased patch to sage-5.7.beta0 (not released yet).
comment:45 Changed 5 years ago by
- Merged in set to sage-5.7.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
1) Please don't use assert, raise ValueError? instead.
2) When returning self._black_list in black_list(), make a copy. i.e. return self._back_list[:]. Otherwise modifications to the returned list will also modify self._black_list.
3)
return self._f(self._inst,*args,**kwds)
has missing white space between the arguments4) In combinatorial_maps_in_class(), you search a unsorted list on every iteration. Either make 'result' a set and possibly convert it into a list on return or if the order matters, use a set next to the list. 5) In the first doc string "defined a map between two combinatorial objects." defined should be defines.
These two classes share a lot of code. Have you though about a way to avoid this duplication?