Opened 10 years ago

Closed 10 years ago

#14052 closed enhancement (fixed)

Enabling plotting of top graded piece of infinite dimensional crystals

Reported by: Anne Schilling Owned by: Sage Combinat CC user
Priority: major Milestone: sage-5.7
Component: combinatorics Keywords: crystals, digraph, plotting, days45
Cc: Sage Combinat CC user Merged in: sage-5.7.rc0
Authors: Anne Schilling Reviewers: Nicolas M. Thiery, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Anne Schilling)

This patch changes the iterator for highest weight crystals to TransitiveIdealGraded? to allow for plotting of the top piece of an infinite dimensional crystal.

Apply trac_14052-infinite-crystal-as.patch

Attachments (2)

trac_14052-infinite-crystal-as.patch (31.0 KB) - added by Anne Schilling 10 years ago.
backtrack-sl.patch (6.2 KB) - added by Sébastien Labbé 10 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 10 years ago by Anne Schilling

Status: newneeds_review

comment:2 in reply to:  1 ; Changed 10 years ago by Anne Schilling

Travis wrote a review patch which has been incorporated. The patch is ready for review!

Anne

Changed 10 years ago by Anne Schilling

comment:3 in reply to:  2 Changed 10 years ago by Anne Schilling

Removed trailing white spaces.

Anne

comment:4 Changed 10 years ago by Travis Scrimshaw

Status: needs_reviewpositive_review

Everything looks good. Thanks.

comment:5 Changed 10 years ago by Sébastien Labbé

Status: positive_reviewneeds_info

Hi Anne,

I have one small comment about the patch related to the modifications made to the file sage/combinat/backtrack.py. I do not think it is a good idea to add this new argument to the __init__ of the TransitiveIdealGraded. I believe the maximal recursion depth should be an argument to the iterator method.

In fact, I started a patch just today in the train to fix stuff about TransitiveIdealGraded and TransitiveIdeal. I believe the creation of the class TransitiveIdealGraded was a mistake. For the same reason, we do not create a class of integers that we are going to multiplicate, another class for integers that we are going to add, and create a class for integers that we are going to factor. And now adding this new argument to the init does not go in the good direction...

I am goind to upload a patch in a few minutes to make my point more clear. Then, I will leave you the choice of using it or not...

Changed 10 years ago by Sébastien Labbé

Attachment: backtrack-sl.patch added

comment:6 Changed 10 years ago by Sébastien Labbé

The patch I just uploaded shows how I think it should be done : adding an argument to the iterator method. My patch should create some conflicts with yours, I am sorry for that. Do you want to rebase your patch on mine?

What my patch do not do for now is to deprecate this TransitiveIdealGraded class. This could be done in another ticket (in #6637 perhaps?). Anyhow, since I believe we should deprecate that class, I also think we should avoid to add new features to it.

comment:7 in reply to:  6 Changed 10 years ago by Nicolas M. Thiéry

Salut Sébastien!

Replying to slabbe:

The patch I just uploaded shows how I think it should be done : adding an argument to the iterator method. My patch should create some conflicts with yours, I am sorry for that. Do you want to rebase your patch on mine?

What my patch do not do for now is to deprecate this TransitiveIdealGraded class. This could be done in another ticket (in #6637 perhaps?). Anyhow, since I believe we should deprecate that class, I also think we should avoid to add new features to it.

Thanks much for working on the refactoring of backtrack.py; we need it!

That being said, TransitiveIdeal? itself is going to get deprecated in the process since we want a consistent naming scheme around RecursiveSet? (that's the name we converged to on the mailing list, right?). So either way, adding an argument to TransitiveIdealGraded? or adding a breath first search iterator in TransitiveIdeal? is also adding features to to-be-deprecated classes.

Also, in the long run, we will want the following features:

(1) RecursiveSet?(generator, operators, max_depth=n)

A parent modeling the top piece of S (all elements of depth <= n)

(2) RecursiveSet?(generator, operators, predicate=...)

A parent modeling an upper ideal of S defined by a predicate

(3) RecursiveSet?(generator, operators, enumeration="depth")

A parent modeling S, with depth first enumeration

(4) RecursiveSet?(generator, operators, enumeration="breath")

A parent modeling S, with breath first enumeration

(5) ...

and not only have iterators for the above. And it's helpful for Anne to have the feature right away for (1).

(note: the exact syntax is another topic of discussion; I don't mean that it should be exactly as above)

So altogether, given that Anne's ticket is ready to go and is important for later development here during the semester, do you mind letting it go as is and rebasing your patch on top of it? We can add a quick note saying that this feature will be refactored (as all the rest).

Cheers,

Nicolas

Last edited 10 years ago by Nicolas M. Thiéry (previous) (diff)

comment:8 Changed 10 years ago by Anne Schilling

Description: modified (diff)

comment:9 Changed 10 years ago by Travis Scrimshaw

Status: needs_infoneeds_review

Hey Sebastien,

I agree with you that there needs to be a more comprehensive class rather than two smaller classes, and I thank you for working on refactoring the code. However I believe that the depth should be an attribute of the class rather than an optional argument for an iterator method because these classes model a (sub)set obtained by (recursive) iteration. I don't know of any other such feature in sage.

Additionally I believe that just because a class is scheduled to be deprecated, this is not a reason to incorporate a useful feature or a short-term fix. I also find this feature helpful for my code (for #13872).

If you want a note that backtrack.py will be refactored, I would be fine with that. However, I would prefer braktrack.py to be refactored after this patch and so I am setting this back to positive review.

Best,
Travis

comment:10 Changed 10 years ago by Travis Scrimshaw

Status: needs_reviewpositive_review

comment:11 Changed 10 years ago by Sébastien Labbé

Salut Anne, Travis and Nicolas,

You convinced me. I will rebase my patch over yours and start the refactorization.

Sébastien

comment:12 Changed 10 years ago by Sébastien Labbé

for the patchbot:

Apply trac_14052-infinite-crystal-as.patch

comment:13 in reply to:  12 Changed 10 years ago by Anne Schilling

Hi Sébastien,

Thank you! If you are indeed going to refactor the backtracker, you probably want to do everything under #6637.

Best,

Anne

comment:14 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.7sage-5.8

comment:15 Changed 10 years ago by Anne Schilling

Keywords: days45 added
Milestone: sage-5.8sage-5.7

comment:16 Changed 10 years ago by Anne Schilling

Milestone: sage-5.7sage-5.8

comment:17 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.8sage-5.7

comment:18 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.7.rc0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.