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

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)

trac_11641-combinatorial_maps_decorator-cs.patch (16.1 KB) - added by jdemeyer 5 years ago.

Download all attachments as: .zip

Change History (46)

comment:1 Changed 6 years ago by stumpc5

  • Description modified (diff)

comment:2 Changed 6 years ago by stumpc5

  • Description modified (diff)

comment:3 Changed 5 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: Changed 5 years ago by 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?

comment:5 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by 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?

comment:6 in reply to: ↑ 5 ; follow-up: Changed 5 years ago by 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)

comment:7 in reply to: ↑ 6 ; follow-up: Changed 5 years ago by 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?

comment:8 in reply to: ↑ 7 Changed 5 years ago by 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.

comment:9 in reply to: ↑ 4 Changed 5 years ago by 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

comment:10 Changed 5 years ago by chrisjamesberg

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

comment:11 follow-up: Changed 5 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: Changed 5 years ago by 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

comment:13 Changed 5 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: Changed 5 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 5 years ago by saliola

  • Status changed from needs_review to needs_info

comment:16 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by 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.

comment:17 in reply to: ↑ 12 Changed 5 years ago by 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.

comment:18 in reply to: ↑ 16 Changed 5 years ago by saliola

  • 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 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: Changed 5 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: Changed 5 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 5 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: Changed 5 years ago by 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

comment:23 in reply to: ↑ 22 Changed 5 years ago by 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

comment:24 Changed 5 years ago by stumpc5

  • 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 chrisjamesberg

  • Status changed from needs_review to positive_review

comment:26 follow-up: Changed 5 years ago by jdemeyer

Why does this have milestone sage-pending?

comment:27 Changed 5 years ago by stumpc5

  • Milestone changed from sage-pending to sage-5.6

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

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 jdemeyer

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

comment:30 follow-up: Changed 5 years ago by stumpc5

  • Status changed from positive_review to needs_work

comment:31 in reply to: ↑ 30 ; follow-up: Changed 5 years ago by 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...

comment:32 in reply to: ↑ 31 Changed 5 years ago by 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.

comment:33 Changed 5 years ago by stumpc5

  • Status changed from needs_work to needs_review

comment:34 follow-up: Changed 5 years ago by jdemeyer

  • Dependencies set to #13074

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

comment:35 in reply to: ↑ 34 ; follow-up: Changed 5 years ago by 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...

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

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: Changed 5 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 5 years ago by 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

comment:39 Changed 5 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 5 years ago by zabrocki

  • Reviewers changed from Chris Berg to Mike Zabrocki

comment:41 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-5.6 to sage-5.7

comment:42 Changed 5 years ago by stumpc5

  • Cc hivert added

comment:43 Changed 5 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 5 years ago by jdemeyer

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

comment:45 Changed 5 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.