Opened 6 years ago
Closed 6 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 )
This patch changes the iterator for highest weight crystals to TransitiveIdealGraded? to allow for plotting of the top piece of an infinite dimensional crystal.
Attachments (2)
Change History (20)
comment:1 follow-up: ↓ 2 Changed 6 years ago by
- Status changed from new to needs_review
comment:2 in reply to: ↑ 1 ; follow-up: ↓ 3 Changed 6 years ago by
Changed 6 years ago by
comment:3 in reply to: ↑ 2 Changed 6 years ago by
Removed trailing white spaces.
Anne
comment:4 Changed 6 years ago by
- Status changed from needs_review to positive_review
Everything looks good. Thanks.
comment:5 Changed 6 years ago by
- 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 6 years ago by
comment:6 follow-up: ↓ 7 Changed 6 years ago by
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 6 years ago by
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
comment:8 Changed 6 years ago by
- Description modified (diff)
comment:9 Changed 6 years ago by
- 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 6 years ago by
- Status changed from needs_review to positive_review
comment:11 Changed 6 years ago by
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: ↓ 13 Changed 6 years ago by
for the patchbot:
Apply trac_14052-infinite-crystal-as.patch
comment:13 in reply to: ↑ 12 Changed 6 years ago by
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 6 years ago by
- Milestone changed from sage-5.7 to sage-5.8
comment:15 Changed 6 years ago by
- Keywords days45 added
- Milestone changed from sage-5.8 to sage-5.7
comment:16 Changed 6 years ago by
- Milestone changed from sage-5.7 to sage-5.8
comment:17 Changed 6 years ago by
- Milestone changed from sage-5.8 to sage-5.7
comment:18 Changed 6 years ago by
- Merged in set to sage-5.7.rc0
- Resolution set to fixed
- Status changed from positive_review to closed
Travis wrote a review patch which has been incorporated. The patch is ready for review!
Anne