Implementation of decorator for combinatorial maps

[stumpc5]

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

[sluther]

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 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?

[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?

[nbruin]

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 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)

[stumpc5]

Replying to nbruin:

Replying to saliola:

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?

[saliola]

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.

[stumpc5]

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

[chrisjamesberg]

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

[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.

[saliola]

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

[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?

[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?

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.

[nthiery]

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.

[saliola]

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 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.

[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

[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.

[stumpc5]

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

[stumpc5]

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

[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

[jdemeyer]

Why does this have milestone sage-pending?

[stumpc5]

Replying to jdemeyer:

Why does this have milestone sage-pending?

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

[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...

[stumpc5]

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.

[jdemeyer]

Could you rebase your patch on top of #13074 please.

[stumpc5]

Replying to jdemeyer:

Could you rebase your patch on top of #13074 please.

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

[stumpc5]

Replying to stumpc5:

Chris, do you think it is worth giving this patch a positive review now?

Cheers, Christian

[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

[stumpc5]

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

[zabrocki]

I've looked this over now, tested it and I give it a positive review. -Mike

[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.

[jdemeyer]

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