Opened 7 years ago

Closed 7 years ago

#14052 closed enhancement (fixed)

Enabling plotting of top graded piece of infinite dimensional crystals

Reported by: aschilling Owned by: sage-combinat
Priority: major Milestone: sage-5.7
Component: combinatorics Keywords: crystals, digraph, plotting, days45
Cc: sage-combinat 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:

Description (last modified by aschilling)

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 aschilling 7 years ago.
backtrack-sl.patch (6.2 KB) - added by slabbe 7 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 follow-up: Changed 7 years ago by aschilling

  • Status changed from new to needs_review

comment:2 in reply to: ↑ 1 ; follow-up: Changed 7 years ago by aschilling

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

Anne

Changed 7 years ago by aschilling

comment:3 in reply to: ↑ 2 Changed 7 years ago by aschilling

Removed trailing white spaces.

Anne

comment:4 Changed 7 years ago by tscrim

  • Status changed from needs_review to positive_review

Everything looks good. Thanks.

comment:5 Changed 7 years ago by slabbe

  • Status changed from positive_review to needs_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 7 years ago by slabbe

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

comment:7 in reply to: ↑ 6 Changed 7 years ago by nthiery

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 7 years ago by nthiery (previous) (diff)

comment:8 Changed 7 years ago by aschilling

  • Description modified (diff)

comment:9 Changed 7 years ago by tscrim

  • Status changed from needs_info to needs_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 7 years ago by tscrim

  • Status changed from needs_review to positive_review

comment:11 Changed 7 years ago by slabbe

Salut Anne, Travis and Nicolas,

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

Sébastien

comment:12 follow-up: Changed 7 years ago by slabbe

for the patchbot:

Apply trac_14052-infinite-crystal-as.patch

comment:13 in reply to: ↑ 12 Changed 7 years ago by aschilling

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 7 years ago by jdemeyer

  • Milestone changed from sage-5.7 to sage-5.8

comment:15 Changed 7 years ago by aschilling

  • Keywords days45 added
  • Milestone changed from sage-5.8 to sage-5.7

comment:16 Changed 7 years ago by aschilling

  • Milestone changed from sage-5.7 to sage-5.8

comment:17 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.8 to sage-5.7

comment:18 Changed 7 years ago by jdemeyer

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