Opened 11 years ago

Closed 9 years ago

Implementation of decorator for combinatorial maps

Reported by: Owned by: stumpc5 sage-combinat major sage-5.7 combinatorics combinatorial statistic chrisjamesberg, saliola, zabrocki, hivert sage-5.7.beta1 Christian Stump Mike Zabrocki N/A #13074

This patch implements decorators for combinatorial maps. The aim is to be able to ask for all combinatorial maps for a given combinatorial class.

comment:1 Changed 11 years ago by stumpc5

• Description modified (diff)

comment:2 Changed 11 years ago by stumpc5

• Description modified (diff)

comment:3 Changed 10 years ago by stumpc5

• Authors changed from Chris Berg, Franco Saliola, Christian Stump to Christian Stump
• 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 10 years ago by sluther

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 arguments
4) 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?

comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 10 years ago by saliola

Thanks for taking a look at the patch and the helpful suggestions. One question:

Why shouldn't one use assert?

comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 10 years ago by nbruin

Thanks for taking a look at the patch and the helpful suggestions. One question:

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 appropriate `try ... 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 10 years ago by stumpc5

Thanks for taking a look at the patch and the helpful suggestions. One question:

Also thanks for looking at it!

@Chris, Franco: are you going to address the issues in the review or should I make the changes?

comment:8 in reply to: ↑ 7 Changed 10 years ago by saliola

@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 10 years ago by stumpc5

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 10 years ago by chrisjamesberg

Fixed the bug that was occurring. Didn't have permission to delete your old patch.

comment:11 follow-up: ↓ 12 Changed 10 years ago by 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.

comment:12 in reply to: ↑ 11 ; follow-up: ↓ 17 Changed 10 years ago by saliola

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 10 years ago by chrisjamesberg

• 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 10 years ago by 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

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` or `sage.combinat.combinatorial_maps` instead of `sage.misc`?

comment:15 Changed 10 years ago by saliola

• Status changed from needs_review to needs_info

comment:16 in reply to: ↑ 14 ; follow-up: ↓ 18 Changed 10 years ago by stumpc5

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` or `sage.combinat.combinatorial_maps` instead of `sage.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 10 years ago by nthiery

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 10 years ago by saliola

• Work issues set to move file into combinat tree ; address TODOs in the file

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` or `sage.combinat.combinatorial_maps` instead of `sage.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`.

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 10 years ago by stumpc5

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 10 years ago by chrisjamesberg

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 10 years ago by stumpc5

• 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 10 years ago by stumpc5

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 10 years ago by 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 10 years ago by stumpc5

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 9 years ago by chrisjamesberg

• Status changed from needs_review to positive_review

comment:26 follow-up: ↓ 28 Changed 9 years ago by jdemeyer

Why does this have milestone `sage-pending`?

comment:27 Changed 9 years ago by stumpc5

• Milestone changed from sage-pending to sage-5.6

comment:28 in reply to: ↑ 26 Changed 9 years ago by stumpc5

Why does this have milestone `sage-pending`?

thanks for bringing this up -- changed to 5.6...

comment:29 Changed 9 years ago by jdemeyer

• Reviewers set to Chris Berg
• Work issues move file into combinat tree deleted

comment:30 follow-up: ↓ 31 Changed 9 years ago by stumpc5

• Status changed from positive_review to needs_work

comment:31 in reply to: ↑ 30 ; follow-up: ↓ 32 Changed 9 years ago by 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 9 years ago by 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 9 years ago by stumpc5

• Status changed from needs_work to needs_review

comment:34 follow-up: ↓ 35 Changed 9 years ago by jdemeyer

• Dependencies set to #13074

comment:35 in reply to: ↑ 34 ; follow-up: ↓ 36 Changed 9 years ago by stumpc5

done, thanks for taking care of that! Let's see what the patchbot says...

comment:36 in reply to: ↑ 35 ; follow-up: ↓ 37 Changed 9 years ago by 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 9 years ago by 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?

-Mike

comment:38 in reply to: ↑ 37 Changed 9 years ago by stumpc5

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 9 years ago by zabrocki

• 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 9 years ago by zabrocki

• Reviewers changed from Chris Berg to Mike Zabrocki

comment:41 Changed 9 years ago by jdemeyer

• Milestone changed from sage-5.6 to sage-5.7

comment:43 Changed 9 years ago by nthiery

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.

comment:44 Changed 9 years ago by jdemeyer

Rebased patch to sage-5.7.beta0 (not released yet).

comment:45 Changed 9 years ago by jdemeyer

• Merged in set to sage-5.7.beta1
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.