Enabling plotting of top graded piece of infinite dimensional crystals

[anneschilling]

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 attachment: trac_14052-infinite-crystal-as.patch

CC: @sagetrac-sage-combinat

Component: combinatorics

Keywords: crystals, digraph, plotting, days45

Author: Anne Schilling

Reviewer: Nicolas M. Thiery, Travis Scrimshaw

Merged: sage-5.7.rc0

Issue created by migration from https://trac.sagemath.org/ticket/14052

[anneschilling]

comment:2

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

Anne

[anneschilling]

comment:3

Attachment: trac_14052-infinite-crystal-as.patch.gz

Removed trailing white spaces.

Anne

[tscrim]

comment:4

Everything looks good. Thanks.

[seblabbe]

comment:5

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

[seblabbe]

Attachment: backtrack-sl.patch.gz

[seblabbe]

comment:6

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.

[nthiery]

comment:7

Salut Sébastien!

Replying to @seblabbe:

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

[tscrim]

comment:9

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

[seblabbe]

comment:11

Salut Anne, Travis and Nicolas,

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

Sébastien

[seblabbe]

comment:12

for the patchbot:

Apply trac_14052-infinite-crystal-as.patch

[anneschilling]

comment:13

Hi Sébastien,

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

Best,

Anne

[anneschilling]

Changed keywords from crystals, digraph, plotting to crystals, digraph, plotting, days45

[jdemeyer]

Merged: sage-5.7.rc0