Opened 6 years ago

Closed 4 years ago

#10527 closed enhancement (fixed)

Implementation of quiver mutation type

Reported by: stumpc5 Owned by: sage-combinat
Priority: major Milestone: sage-5.5
Component: combinatorics Keywords: quiver mutation type days38
Cc: musiker@…, robertwb Merged in: sage-5.5.beta0
Authors: Christian Stump Reviewers: Hugh Thomas
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by stumpc5)

The patch implements multiple mutation types of quivers. This classification contains in particular all finite and affine types of generalized Cartan types.

The patch contains multiple examples.

sage: mt = QuiverMutationType(['A',3]); mt
['A', 3]

Apply only

  • trac_10527-quiver_mutation_type.patch

Attachments (1)

trac_10527-quiver_mutation_type.patch (75.8 KB) - added by jdemeyer 4 years ago.

Download all attachments as: .zip

Change History (134)

comment:1 Changed 6 years ago by stumpc5

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by stumpc5

  • Dependencies set to #10349

comment:3 follow-up: Changed 6 years ago by chapoton

This one contains "attach" statements, which seems very strange and probably causes many problems.

The header is also bad.

And QuiverMutationType? is not recognized as a global command.

comment:4 follow-up: Changed 6 years ago by chapoton

There also remains some "hyperbolic" that should really be "elliptic".

comment:5 in reply to: ↑ 4 Changed 6 years ago by stumpc5

Replying to chapoton:

There also remains some "hyperbolic" that should really be "elliptic".

done!

comment:6 in reply to: ↑ 3 Changed 6 years ago by stumpc5

Replying to chapoton:

This one contains "attach" statements, which seems very strange and probably causes many problems.

Somehow, all my changes belonging to this patch got into the next on cluster algebras and quivers...

On my computer, everything seems to be fine after moving all these changes into this patch, and all doctests pass.

comment:7 Changed 6 years ago by stumpc5

  • Milestone set to sage-4.7.1

comment:8 Changed 6 years ago by stumpc5

  • Milestone changed from sage-4.7.1 to sage-4.7.2

comment:9 follow-up: Changed 5 years ago by chapoton

  • Status changed from needs_review to needs_work

There seems to be some "trailing whitespace" problem to be solved. This should be easy enough, I guess.

comment:10 in reply to: ↑ 9 Changed 5 years ago by stumpc5

  • Status changed from needs_work to needs_review

Replying to chapoton:

There seems to be some "trailing whitespace" problem to be solved. This should be easy enough, I guess.

should be fixed - thanks for looking at the ticket!

comment:11 Changed 5 years ago by chapoton

  • Status changed from needs_review to needs_work

comment:12 follow-up: Changed 5 years ago by hthomas

What did you mean by:

It appears that C(M) and C(M') are isomorphic Cartan types for mutation equivalent skew-symmetrizable matrices M and M'.

A3 is mutation equivalent to an oriented cycle, whose Cartan type is affine A2. Maybe this should be removed?

(I'm at Sage Days 38, and I'm planning to get a bunch of refereeing done this week, so I'll probably have more comments....)

comment:13 follow-up: Changed 5 years ago by hthomas

It seems that this patch and 10538 depend on each other.

sage: QuiverMutationType('X',6,2).standard_quiver()

/home/hugh/sage-5.0.rc0/local/lib/python2.7/site-packages/sage/combinat/cluster_algebra_quiver/quiver_mutation_type.pyc in standard_quiver(self)
   1439             Quiver on 12 vertices of type [ ['A', 3], ['B', 3], ['X', 6, 2] ]
   1440         """
-> 1441         from quiver import Quiver
   1442         Q = Quiver( self._digraph )
   1443         Q._mutation_type = self

ImportError: No module named quiver

I could go ahead and (try to) review both together, unless you have a better suggestion.

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

Replying to hthomas:

What did you mean by:

It appears that C(M) and C(M') are isomorphic Cartan types for mutation equivalent skew-symmetrizable matrices M and M'.

A3 is mutation equivalent to an oriented cycle, whose Cartan type is affine A2. Maybe this should be removed?

(I'm at Sage Days 38, and I'm planning to get a bunch of refereeing done this week, so I'll probably have more comments....)

agreed.

comment:15 in reply to: ↑ 13 Changed 5 years ago by stumpc5

Replying to hthomas:

It seems that this patch and 10538 depend on each other. I could go ahead and (try to) review both together, unless you have a better suggestion.

Hi Hugh,

If I am not mistaken, this is the only dependency of this patch on #10538. What about the following:

  • we take this method off this patch and add it to the other
  • you can review of this patch (I guess, we might need some time for that already)
  • you can spend more time on the other patch. It would be great, if we can get it reviewed this week, but this will be far from easy...

Thanks, Christian

comment:16 in reply to: ↑ 14 ; follow-ups: Changed 5 years ago by hthomas

Replying to stumpc5:

Replying to hthomas:

What did you mean by:

It appears that C(M) and C(M') are isomorphic Cartan types for mutation equivalent skew-symmetrizable matrices M and M'.

A3 is mutation equivalent to an oriented cycle, whose Cartan type is affine A2. Maybe this should be removed?

(I'm at Sage Days 38, and I'm planning to get a bunch of refereeing done this week, so I'll probably have more comments....)

agreed.

Okay. I'm working on a reviewer patch, so I can take it out there.

comment:17 in reply to: ↑ 16 Changed 5 years ago by stumpc5

Replying to hthomas:

Replying to stumpc5:

Replying to hthomas:

Okay. I'm working on a reviewer patch, so I can take it out there.

could you please use the patches in the combinat queue, since I am certain that they are up-to-date, while I lost track over the months if I always updated the version on the trac server.

Thanks, Christian

comment:18 follow-up: Changed 5 years ago by hthomas

Hi Christian--

Sure, I can use the combinat patches. Is that going to fix the dependency loop? I didn't quite gather what you wanted to move into the second patch: both plot() and show() use standard_quiver().

cheers,

Hugh

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

Sure, I can use the combinat patches. Is that going to fix the dependency loop? I didn't quite gather what you wanted to move into the second patch: both plot() and show() use standard_quiver().

have you already touched the files? If you give me 15min, I will remove the dependency loop and push.

Last edited 5 years ago by stumpc5 (previous) (diff)

comment:20 in reply to: ↑ 19 ; follow-up: Changed 5 years ago by hthomas

Replying to stumpc5:

Sure, I can use the combinat patches. Is that going to fix the dependency loop? I didn't quite gather what you wanted to move into the second patch: both plot() and show() use standard_quiver().

have you already touched the files? If you give me 15min, I will remove the dependency loop and push.

That'll be great. Thanks.

comment:21 in reply to: ↑ 20 Changed 5 years ago by stumpc5

That'll be great. Thanks.

I pushed a new version to the combinat queue.

A small issue I see is that the internal functions on the bottom are documented, but the documentations don't contain any tests.

If you have any questions, or if you need help, please let me know!

comment:22 Changed 5 years ago by hthomas

Thanks!

comment:23 follow-up: Changed 5 years ago by hthomas

Hi Christian--

I think QuiverMutationType(BB,1,1) shouldn't return affine A1. They are double edges in different senses. (Affine A1 is two arrows in the quiver convention, while I think the only reasonable interpretation of BB,1,1 is a double edge in the Lie convention, i.e. a valued quiver with one edge and labels (1,2). That is to say, I think BB,1,1 should return B2.)

cheers,

Hugh

comment:24 in reply to: ↑ 23 Changed 5 years ago by stumpc5

I think QuiverMutationType(BB,1,1) shouldn't return affine A1. They are double edges in different senses. (Affine A1 is two arrows in the quiver convention, while I think the only reasonable interpretation of BB,1,1 is a double edge in the Lie convention, i.e. a valued quiver with one edge and labels (1,2). That is to say, I think BB,1,1 should return B2.)

What you say is right, but I am also not very convinced of returning B2: I don't like that the BBn-1 and Bn turn into B2 for n=2, since the BB tells that there are two special outgoing edges. (Same for CCn-1 as well.) BCn-1 turns into one edge (1,-4) which seems to be more appropriate. What about not allowing BB1 and CC1 as input, and starting both series with n=2? We actually do the same with BDn-1, where we start only with n=3 since one end is special in the B sense and the other in the D sense.

comment:25 follow-ups: Changed 5 years ago by hthomas

  • Description modified (diff)
  • Keywords days38 added
  • Reviewers set to Hugh Thomas
  • Status changed from needs_work to needs_review

Okay, I agree, BB,1,1 should be undefined.


I'm confused about the notation you're using for some of the exceptional finite mutation type classes. The non-simply-laced finite mutation type mutation classes are classified in Felikson-Shapiro-Tumarkin, arXiv:1006.4276. They are elliptic root systems (F-S-T say this), contrary to what's in the package now, and as a result I would rather get them using Saito notation. But I don't see how the data for them match up with the data in F-S-T.


Here's another question: for the most part, the third parameter is redundant. I think the third parameter should only be used for the Kac convention (in affine root systems) and the Saito convention (in elliptic root systems).

The only other information you're using it for is when you want to indicate the check in the other convention for affine roots. But "check=true" would be easier to remember.

I also think it might be helpful to indicate references for the different conventions.

http://en.wikipedia.org/wiki/Dynkin_diagram#Affine_Dynkin_diagrams

seems to be a pretty good reference for the affine diagrams using the Kac convention. And the convention Macdonald uses is recorded online at

http://assets.cambridge.org/97805218/24729/excerpt/9780521824729_excerpt.pdf

Then there is possibly a more canonical reference for Saito than F-S-T.


It seems potentially confusing that "rank" is an argument for creating the object and also an attribute of the resulting object, but they aren't equal. I guess all I'm really asking for is that the parameter for creating the object should have a different name.


I haven't heard of types AE, BE, CE, DE before. BE, CE, and DE seem to be constructed on the same logic as BC, BD, etc, so it's pretty clear what they're doing, but I'm not sure that they're needed. AE seems to be constructed on a rather different logic; logically (to me) AE should just refer to the E-series. So in addition to wondering if it's needed, I'm not very happy with the name.

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

  • Cc musiker@… added

Replying to hthomas:

Okay, I agree, BB,1,1 should be undefined.

okay -- are you going to edit this patch, or (even better) add a review patch right after?


I'm confused about the notation you're using for some of the exceptional finite mutation type classes. The non-simply-laced finite mutation type mutation classes are classified in Felikson-Shapiro-Tumarkin, arXiv:1006.4276. They are elliptic root systems (F-S-T say this), contrary to what's in the package now, and as a result I would rather get them using Saito notation. But I don't see how the data for them match up with the data in F-S-T.

I think you know more about that than I do: are you saying that types V,W,X,Y,Z are elliptic?


Here's another question: for the most part, the third parameter is redundant. I think the third parameter should only be used for the Kac convention (in affine root systems) and the Saito convention (in elliptic root systems).

Yes, that's right. The reason we still use it is in order to make it a priori clear for the user what kind of type he/she is using: None for finite 1 for affine, 2 for mutation finite, and 3 for mutation infinite. But if you think that's too much, or not needed, we can as well delete it. You can maybe ask for another opinion on that at the sage days.


I also think it might be helpful to indicate references for the different conventions.

http://en.wikipedia.org/wiki/Dynkin_diagram#Affine_Dynkin_diagrams

seems to be a pretty good reference for the affine diagrams using the Kac convention. And the convention Macdonald uses is recorded online at

http://assets.cambridge.org/97805218/24729/excerpt/9780521824729_excerpt.pdf

Then there is possibly a more canonical reference for Saito than F-S-T.

agreed with all these.


It seems potentially confusing that "rank" is an argument for creating the object and also an attribute of the resulting object, but they aren't equal. I guess all I'm really asking for is that the parameter for creating the object should have a different name.

can you think of a good name? choose whatever you find appropriate.


I haven't heard of types AE, BE, CE, DE before. BE, CE, and DE seem to be constructed on the same logic as BC, BD, etc, so it's pretty clear what they're doing, but I'm not sure that they're needed.

me neither, but I discussed that with Gregg, and we thought that we just include them. If you really think no one will ever try to use them, then we can as well delete them.

AE seems to be constructed on a rather different logic; logically (to me) AE should just refer to the E-series. So in addition to wondering if it's needed, I'm not very happy with the name.

I think of AE as an affine type A plus an extra edge, so the logic is still the same (though the only point where it is visible that we have affine type A is that we use (a,b) as a second parameter in order to say how many edges are oriented in which direction.


I added Gregg as cc, so he can also give his opinion...

comment:27 in reply to: ↑ 26 ; follow-up: Changed 5 years ago by hthomas

Replying to stumpc5:

Replying to hthomas:

Okay, I agree, BB,1,1 should be undefined.

okay -- are you going to edit this patch, or (even better) add a review patch right after?

Yes, I am workiing on a review patch. I can do this there.


I'm confused about the notation you're using for some of the exceptional finite mutation type classes. The non-simply-laced finite mutation type mutation classes are classified in Felikson-Shapiro-Tumarkin, arXiv:1006.4276. They are elliptic root systems (F-S-T say this), contrary to what's in the package now, and as a result I would rather get them using Saito notation. But I don't see how the data for them match up with the data in F-S-T.

I think you know more about that than I do: are you saying that types V,W,X,Y,Z are elliptic?

X_6 and X_7 (The Derksen-Owen quivers, which are skew-symmetric) are not elliptic. But the sporadic non-skew-symmetric mutation-finite valued quivers are elliptic. This means they have elliptic-sounding names (eg G(1,3)). However, the elliptic-looking quivers are different from the ones which you've provided. Do you want to use the quivers you provided, or the elliptic-looking ones in (for example) FST? Also, do you have a reference for the quivers you provided?


Here's another question: for the most part, the third parameter is redundant. I think the third parameter should only be used for the Kac convention (in affine root systems) and the Saito convention (in elliptic root systems).

Yes, that's right. The reason we still use it is in order to make it a priori clear for the user what kind of type he/she is using: None for finite 1 for affine, 2 for mutation finite, and 3 for mutation infinite. But if you think that's too much, or not needed, we can as well delete it. You can maybe ask for another opinion on that at the sage days.

Okay, I'll get a second opinion. But note that the system you are using is actually less straightforward than that, since if, for example, you invoke a finite mutation type via a family like T, you have to enter a 3 anyhow.


I also think it might be helpful to indicate references for the different conventions.

http://en.wikipedia.org/wiki/Dynkin_diagram#Affine_Dynkin_diagrams

seems to be a pretty good reference for the affine diagrams using the Kac convention. And the convention Macdonald uses is recorded online at

http://assets.cambridge.org/97805218/24729/excerpt/9780521824729_excerpt.pdf

Then there is possibly a more canonical reference for Saito than F-S-T.

agreed with all these.

Great. If people are using Macdonald notation, how do you feel about making them say "affine=true" for affines (if we now reserve "twist" for Kac and Saito notations)?


It seems potentially confusing that "rank" is an argument for creating the object and also an attribute of the resulting object, but they aren't equal. I guess all I'm really asking for is that the parameter for creating the object should have a different name.

can you think of a good name? choose whatever you find appropriate.

Maybe "numerical_data"?


I haven't heard of types AE, BE, CE, DE before. BE, CE, and DE seem to be constructed on the same logic as BC, BD, etc, so it's pretty clear what they're doing, but I'm not sure that they're needed.

me neither, but I discussed that with Gregg, and we thought that we just include them. If you really think no one will ever try to use them, then we can as well delete them.

AE seems to be constructed on a rather different logic; logically (to me) AE should just refer to the E-series. So in addition to wondering if it's needed, I'm not very happy with the name.

I think of AE as an affine type A plus an extra edge, so the logic is still the same (though the only point where it is visible that we have affine type A is that we use (a,b) as a second parameter in order to say how many edges are oriented in which direction.

I guess it does no harm to include BE, CE, DE. The reason AE's notation seems different to me is that BE, CE, DE have "B on one end, E on the other", while this is not really true for AE (the A is really affine A, not finite A, and there isn't anything very E-like about adding an edge at an arbitrary point). I'm inclined to remove it.


I added Gregg as cc, so he can also give his opinion...

Hi Gregg! Please do feel free to weigh in.

comment:28 in reply to: ↑ 27 ; follow-up: Changed 5 years ago by hthomas

Replying to hthomas:

Replying to stumpc5:

Here's another question: for the most part, the third parameter is redundant. I think the third parameter should only be used for the Kac convention (in affine root systems) and the Saito convention (in elliptic root systems).

Yes, that's right. The reason we still use it is in order to make it a priori clear for the user what kind of type he/she is using: None for finite 1 for affine, 2 for mutation finite, and 3 for mutation infinite. But if you think that's too much, or not needed, we can as well delete it. You can maybe ask for another opinion on that at the sage days.

Okay, I'll get a second opinion. But note that the system you are using is actually less straightforward than that, since if, for example, you invoke a finite mutation type via a family like T, you have to enter a 3 anyhow.

Chris Berg agreed with me when I talked to him just now, and Nicolas also agreed with me last night (though I guess I didn't put the question quite as precisely to Nicolas).

Also, Nicolas suggested that the names of the types be more obvious. It's a matter of style, I guess, whether you prefer R2 or Rank2, TR or Triangle, GR or Grassmannian. I think the latter is more inviting to someone who is unfamiliar with the code. But I don't really have a strong feeling about this. (Nicolas's suggestion was based on a quick description of the situation from me, so it could be that he would have a different opinion if he looked into it himself.)

comment:29 Changed 5 years ago by hthomas

_construct_mutation_classes_by_rank, _construct_exceptional_mutation_classes, and _save_exceptional_data_dig6 depend on #10538. Perhaps they could move into there? (This was not detected by the doctesting because, um, there are no doctests for those procedures.)

Also, I am a bit confused about what the role of the _save_... method is. It's a private method, so it in principle shouldn't be called by the user, but it also doesn't seem to be called by the code in this patch or in 10538.

Last edited 5 years ago by hthomas (previous) (diff)

comment:30 in reply to: ↑ 16 ; follow-up: Changed 5 years ago by gmoose05

Replying to hthomas:

Replying to stumpc5:

Replying to hthomas:

What did you mean by:

It appears that C(M) and C(M') are isomorphic Cartan types for mutation equivalent skew-symmetrizable matrices M and M'.

A3 is mutation equivalent to an oriented cycle, whose Cartan type is affine A2. Maybe this should be removed?

I agree that this line appears to be confusing. Is this in the documentation or in the code itself?

(I'm at Sage Days 38, and I'm planning to get a bunch of refereeing done this week, so I'll probably have more comments....)

Sounds good Hugh, Thanks!

agreed.

Okay. I'm working on a reviewer patch, so I can take it out there.

comment:31 in reply to: ↑ 25 Changed 5 years ago by gmoose05

Replying to hthomas:

Okay, I agree, BB,1,1 should be undefined.

Agreed.


I'm confused about the notation you're using for some of the exceptional finite mutation type classes. The non-simply-laced finite mutation type mutation classes are classified in Felikson-Shapiro-Tumarkin, arXiv:1006.4276. They are elliptic root systems (F-S-T say this), contrary to what's in the package now, and as a result I would rather get them using Saito notation. But I don't see how the data for them match up with the data in F-S-T.


Here's another question: for the most part, the third parameter is redundant. I think the third parameter should only be used for the Kac convention (in affine root systems) and the Saito convention (in elliptic root systems).

The only other information you're using it for is when you want to indicate the check in the other convention for affine roots. But "check=true" would be easier to remember.

We were trying to have some notation that would be compact yet flexible. Part of the issue, that there will be users from the Kac convention, but then users from other conventions. Also, since cluster algebra mutation types can be finer than Dynkin types (e.g. in the affine A case, when the number of oriented arrows in a given direction matters) it wasn't a perfect fit either to use the standard Kac conventions.

One ambiguity might be that we wrote that code just after Felikson-Shapiro-Tumarkin's second paper hit the arxiv. They were using different notation for their exceptional examples in 2010 (see version 1-3) than they are in 2011. I am happy for the sage convention to be updated to the 2011 (FST arxiv version 4) notation.

I also think it might be helpful to indicate references for the different conventions.

http://en.wikipedia.org/wiki/Dynkin_diagram#Affine_Dynkin_diagrams

seems to be a pretty good reference for the affine diagrams using the Kac convention. And the convention Macdonald uses is recorded online at

http://assets.cambridge.org/97805218/24729/excerpt/9780521824729_excerpt.pdf

Then there is possibly a more canonical reference for Saito than F-S-T.


It seems potentially confusing that "rank" is an argument for creating the object and also an attribute of the resulting object, but they aren't equal. I guess all I'm really asking for is that the parameter for creating the object should have a different name.


I haven't heard of types AE, BE, CE, DE before. BE, CE, and DE seem to be constructed on the same logic as BC, BD, etc, so it's pretty clear what they're doing, but I'm not sure that they're needed. AE seems to be constructed on a rather different logic; logically (to me) AE should just refer to the E-series. So in addition to wondering if it's needed, I'm not very happy with the name.

I also would vote for keeping BE, CE, DE, but am fine with jettisoning AE.

comment:32 in reply to: ↑ 28 Changed 5 years ago by gmoose05

Replying to hthomas:

Replying to hthomas:

Replying to stumpc5:

Here's another question: for the most part, the third parameter is redundant. I think the third parameter should only be used for the Kac convention (in affine root systems) and the Saito convention (in elliptic root systems).

Yes, that's right. The reason we still use it is in order to make it a priori clear for the user what kind of type he/she is using: None for finite 1 for affine, 2 for mutation finite, and 3 for mutation infinite. But if you think that's too much, or not needed, we can as well delete it. You can maybe ask for another opinion on that at the sage days.

Okay, I'll get a second opinion. But note that the system you are using is actually less straightforward than that, since if, for example, you invoke a finite mutation type via a family like T, you have to enter a 3 anyhow.

Chris Berg agreed with me when I talked to him just now, and Nicolas also agreed with me last night (though I guess I didn't put the question quite as precisely to Nicolas).

Also, Nicolas suggested that the names of the types be more obvious. It's a matter of style, I guess, whether you prefer R2 or Rank2, TR or Triangle, GR or Grassmannian. I think the latter is more inviting to someone who is unfamiliar with the code. But I don't really have a strong feeling about this. (Nicolas's suggestion was based on a quick description of the situation from me, so it could be that he would have a different opinion if he looked into it himself.)

I am fine with using Rank2, Triangle, Grassmannian for the type names if it is more instructive to a user, but if it seems reasonable, would also keep the shorter 'R2', 'TR', and 'GR' as shorter inputs that then coerce to the longer names.

Regarding the issue of the "twist" parameter which is either 'None' for finite, '1' for affine, '2' for mutation-finite, or '3' for other, I am open to suggestion.

We definitely want to distinguish Cartan finite from Cartan affine, or the elliptic types, and wanted some other useful sequences.

In future implementations, we foresaw including other quiver sequences such as the Generalized Glick quivers, Somos 4 and 5 quivers (or other Gale-Robinson sequence quivers) or those coming from a Q-system, and quivers from other surfaces, so we would want an infrastructure that easily allows a user to encode quiver types from certain families. This was part of the mutation for including a twist parameter but maybe its extraneous.

comment:33 in reply to: ↑ 30 ; follow-up: Changed 5 years ago by hthomas

The wierd sentence about Cartan types was in the docstring at the top of the file. I've taken it out.

As I started to think more about how I would prefer to handle affine types, I started to see the advantage of the "negative twist" notation. So I'm coming around to your choice on those.

comment:34 in reply to: ↑ 33 Changed 5 years ago by stumpc5

Replying to hthomas:

As I started to think more about how I would prefer to handle affine types, I started to see the advantage of the "negative twist" notation. So I'm coming around to your choice on those.

Alright, please also pull my changes (I hope that we don't have any conflicts in the merge!).

We can maybe have a quick skype session later today or tomorrow...

Best, Christian

comment:35 follow-up: Changed 5 years ago by hthomas

I added a bunch of little patches. If you'd rather, I can fold them all in together, but I think it might be easier for you to look at them this way. I tried to make each patch deal with one issue (though the first one includes several). I think the doctests pass properly if you apply any initial subsequence of the list of patches.

I am still working, but I thought you might like to see where I'd got to.

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

Replying to hthomas:

I added a bunch of little patches. If you'd rather, I can fold them all in together, but I think it might be easier for you to look at them this way. I tried to make each patch deal with one issue (though the first one includes several). I think the doctests pass properly if you apply any initial subsequence of the list of patches.

I am still working, but I thought you might like to see where I'd got to.

thanks for your work - it looks like we are getting somewhere! I imported and folded all your patches into trac_10527-quiver_mutation_type-ht.patch, and added it to the combinat queue.

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

Replying to stumpc5:

thanks for your work - it looks like we are getting somewhere! I imported and folded all your patches into trac_10527-quiver_mutation_type-ht.patch, and added it to the combinat queue.

I can't find it in the queue. Could you please check that it is really there? Sorry not to have gotten this finished during days38; I couldn't stick around for the last day, and had some other stuff to take care of also. In any case, I hope to get this actually finished soon!

comment:38 in reply to: ↑ 37 Changed 5 years ago by stumpc5

I can't find it in the queue. Could you please check that it is really there? Sorry not to have gotten this finished during days38; I couldn't stick around for the last day, and had some other stuff to take care of also. In any case, I hope to get this actually finished soon!

Sorry, forgot to push - It's there now...

comment:39 follow-up: Changed 5 years ago by hthomas

Thanks, Christian.

For methods which are applicable to either reducible or irreducible types, would it be better to implement them in the class from which both reducible and irreducible inherit? It seems tidier to me. But I don't really have a great global feel for the implications.

It also seems to me that some of the methods which are presently implemented for irreducible and not for reducible could naturally be implemented for both (eg, B-matrix, Cartan matrix). Though I guess some of the "is_xxx" methods don't really make sense in application to a reducible class. (Eg, in some respects, the product of a dynkin and an affine is more affine than the product of two affines is, but I wouldn't want to try to make such rules precise. Some, however, make perfect sense.)

comment:40 follow-up: Changed 5 years ago by hthomas

Christian, could you please explain to me the distinction between QuiverMutationTypeFactory and QuiverMutationType? Sorry, I am very much a beginning when it comes to object-oriented stuff like classes.

comment:41 follow-up: Changed 5 years ago by hthomas

In the patch as it existed before I started messing about with it, there were three things that all had the same docstring: QuiverMutationType?, QuiverMutationTypeFactory?, and the whole document. The same text occurred twice in the patch, once for the whole document and once for QMTF, with QMT getting assigned the docstring for the entire file.

I think the docstring for the whole file should be different from the docstring for QMT. One is only going to see that docstring if one is reading the whole file in the reference manual (right?) so the documentation for QMTF/QMT will be right there. Therefore, I think it makes sense to have the docstring for the whole file be shorter.

I think the docstring for QMT and QMTF don't really have to be duplicated, either -- QMTF could just say "see QMT for the detailed documentation" since it's QMT that people are likely to be looking up.

However, I guess I don't know how to make a docstring for something like QMT, which isn't defined via a def or a class.

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

Hi Hugh --

For methods which are applicable to either reducible or irreducible types, would it be better to implement them in the class from which both reducible and irreducible inherit? It seems tidier to me. But I don't really have a great global feel for the implications. It also seems to me that some of the methods which are presently implemented for irreducible and not for reducible could naturally be implemented for both (eg, B-matrix, Cartan matrix). Though I guess some of the "is_xxx" methods don't really make sense in application to a reducible class. (Eg, in some respects, the product of a dynkin and an affine is more affine than the product of two affines is, but I wouldn't want to try to make such rules precise. Some, however, make perfect sense.)

!QuiverMutationType_Reducible inherits from !QuiverMutationType_Irreducible, so all methods not implemented there, are gotten automatically from the irreducible case. In the reducible case, I only implemented the methods that are different in both casses. Does this make sense to you?

comment:43 in reply to: ↑ 40 Changed 5 years ago by stumpc5

Replying to hthomas:

Christian, could you please explain to me the distinction between QuiverMutationTypeFactory and QuiverMutationType? Sorry, I am very much a beginning when it comes to object-oriented stuff like classes.

I basically took this idea from Nicolas' implementation of CartanType: CartanTypeFactory just delegates different input to the right place -- we have an object

QuiverMutationType = QuiverMutationType()

and if the input is irreducible, we return a !QuiverMutationType_irreducible object and if the input is reducible we return a !QuiverMutationtype_reducible object. Moreover, QuiverMutationType also provides samples of quiver mutation types, see the samples methods in the factory.

PS: I just see that the docstrings for samples and _samples are still saying Cartan type instead of QuiverMutationType?, could you replace that while you're on the file -- thanks!

Last edited 5 years ago by stumpc5 (previous) (diff)

comment:44 in reply to: ↑ 41 Changed 5 years ago by stumpc5

Replying to hthomas:

In the patch as it existed before I started messing about with it, there were three things that all had the same docstring: QuiverMutationType?, QuiverMutationTypeFactory?, and the whole document. The same text occurred twice in the patch, once for the whole document and once for QMTF, with QMT getting assigned the docstring for the entire file.

I think the docstring for the whole file should be different from the docstring for QMT. One is only going to see that docstring if one is reading the whole file in the reference manual (right?) so the documentation for QMTF/QMT will be right there. Therefore, I think it makes sense to have the docstring for the whole file be shorter.

I think the docstring for QMT and QMTF don't really have to be duplicated, either -- QMTF could just say "see QMT for the detailed documentation" since it's QMT that people are likely to be looking up.

However, I guess I don't know how to make a docstring for something like QMT, which isn't defined via a def or a class.

I just looked again how this is handled for Cartan types, and there as well, I don't find the docstring behaviour very satisfying. I post your question on sage-combinat-devel, so we can discuss that in public.

PS: this is now https://groups.google.com/d/topic/sage-combinat-devel/4TZMw3SXQPk/discussion

Last edited 5 years ago by stumpc5 (previous) (diff)

comment:45 in reply to: ↑ 42 ; follow-up: Changed 5 years ago by nthiery

Replying to stumpc5:

!QuiverMutationType_Reducible inherits from !QuiverMutationType_Irreducible

Just my 2 cents: this is an abuse since a reducible cartan type is not an irreducible cartan type (http://en.wikipedia.org/wiki/Is-a). It would be preferable to introduce an upper class from which both would derive.

Cheers,

Nicolas

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

Just my 2 cents: this is an abuse since a reducible cartan type is not an irreducible cartan type (http://en.wikipedia.org/wiki/Is-a). It would be preferable to introduce an upper class from which both would derive.

@Nicolas: Thanks for the comments (here and on sage-combinat-devel).

@Hugh: Could you push the newest version you have, I will then make the reorganization (which is hopefully straightforward) of the classes, and of the docstring.

comment:47 in reply to: ↑ 46 Changed 5 years ago by hthomas

Replying to stumpc5:

@Hugh: Could you push the newest version you have, I will then make the reorganization (which is hopefully straightforward) of the classes, and of the docstring.

I have not made any new changes.

Christian, what do you think of the idea of having a cartan_type method which would produce a CartanType from a QuiverMutationType? It seems to me that this could reduce the amount of cluster-algebra-specific code, since a bunch of things are implemented for cartan_type. And this would be better in the long run, as cartan_type is improved.

I'm sorry about the number of non-trivial changes I am suggesting!

Last edited 5 years ago by hthomas (previous) (diff)

comment:48 Changed 5 years ago by stumpc5

I reorganized it in the way that we now have QMT_abstract containing all methods for both, and then QMT_irreducible / QMT_reducible both inheriting form QMT_abstract. I also like the docstring better now: QMT? gives the long docstring, QMT?? gives the source with a reference to the long docstring, and then also the long docstring.

Concerning a potential cartan_type method: what do you want to return if I have a quiver mutation type for which no Cartan type exists, I think of QuiverMutationType('GR',(3,10)) for example?

comment:49 follow-ups: Changed 5 years ago by hthomas

I think the right thing to do is to implement a CartanType for general root data -- the general version would probably mainly know about its Cartan matrix, for starters, but eventually you'd want it to know about the associated Weyl group, for example.

Nicolas, what do you think? (Or since I'm not sure Nicolas will be following this ticket closely, and in any case there might be other interested people, maybe we should take this discussion to sage-combinat-devel?)

comment:50 in reply to: ↑ 49 Changed 5 years ago by nthiery

Replying to hthomas:

I think the right thing to do is to implement a CartanType for general root data -- the general version would probably mainly know about its Cartan matrix, for starters, but eventually you'd want it to know about the associated Weyl group, for example.

Nicolas, what do you think? (Or since I'm not sure Nicolas will be following this ticket closely, and in any case there might be other interested people, maybe we should take this discussion to sage-combinat-devel?)

I am following :-) But I am not enough of a user on that topic to have a good view on the use cases. Of course it seems that the more information is shared with usual Cartan type's, the better it is.

Cheers,

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

I think the right thing to do is to implement a CartanType for general root data -- the general version would probably mainly know about its Cartan matrix, for starters, but eventually you'd want it to know about the associated Weyl group, for example.

Hi Hugh --

I recently implemented a CartanMatrix (available in the patch on reflection groups). I now added a patch

trac_10527-quiver_mutation_type-ht.patch
trac_???-cartan_matrix_for_quiver_mutation_type-cs.patch

and now, we have the following behaviour:

sage: mt = QuiverMutationType(['A',3])      
sage:  C = mt.cartan_matrix()              
sage:  G = C.reflection_group()
sage:  G.cardinality()
24

sage: mt = QuiverMutationType(['GR',(10,3)])
sage:  C = mt.cartan_matrix()
sage:  G = C.reflection_group()
sage:  G.cardinality()
+Infinity

Sweet, he? Beside that, the class CartanMatrix is not yet very advanced, and several important decisions are still to be made (e.g., how should CartanMatrix, CartanType, RootSystem, RootSpace, and so on, interact?), but that's a different topic.

How do you think to change the organization of the methods by using CartanMatrix (or CartanType)?

Best, Christian

comment:52 in reply to: ↑ 51 Changed 5 years ago by nthiery

Sweet, he?

Pretty sweet indeed!

Beside that, the class CartanMatrix is not yet very advanced,

and several important decisions are still to be made (e.g., how should CartanMatrix, CartanType, RootSystem, RootSpace, and so on, interact?), but that's a different topic.

How do you think to change the organization of the methods by using CartanMatrix (or CartanType)?

Having cartan_type return a Cartan matrix would seem just natural to me, as long as all operations on cartan types (building the root system, the weyl group, ...) are available from a cartan matrix (they should). In the long run, we probably want to rename all our cartan_type methods to cartan_datum.

Cheers,

Nicolas

-- Nicolas M. Thiéry "Isil" <nthiery@…> http://Nicolas.Thiery.name/

comment:53 Changed 5 years ago by hthomas

Nice!

I guess my idea was that once you can get from a QuiverMutationType to a Cartan matrix, then things like coxeter_diagram and coxeter_matrix should be implemented as methods for Cartan matrices. Does that make sense?

Maybe it's not really a huge deal. I was maybe imagining that there was more Cartan-related code in quiver_mutation_type than there presently is.

comment:54 follow-up: Changed 5 years ago by hthomas

Christian, by the way, if you wanted to add doctesting for the methods which don't presently have it, that would be very helpful.

comment:55 follow-up: Changed 5 years ago by hthomas

Very minor question: why is the new directory containing all the cluster-algebra related stuff called cluster_algebra_quiver rather than cluster_algebra? I guess you must have had a reason, but I don't quite see what it would have been, and without some reason to do otherwise, I would opt for the shorter and simpler name. (Sorry for the stupid question. I do plan to do something useful as well...)

comment:56 follow-up: Changed 5 years ago by hthomas

Hello, Christian and Nicolas--

The Cartan matrix for finite type B is the same, whether we go via CartanType or QuiverMutationType (good, and not a surprise). However, it is transposed compared to what is in Bourbaki (surprise, and probably not so good). The same thing is true of finite type C.

Is this some difference of choice of convention, or is it a bug?

(I started looking at affine types, because there seems as if there might be some confusion there also, but I guess to begin with, it would be good if I understood the convention in finite type.)

cheers,

Hugh

comment:57 in reply to: ↑ 54 Changed 5 years ago by stumpc5

Replying to hthomas:

Christian, by the way, if you wanted to add doctesting for the methods which don't presently have it, that would be very helpful.

done.

I actually edited your patch, Hugh, so please pull my changes!

Last edited 5 years ago by stumpc5 (previous) (diff)

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

Replying to hthomas:

Very minor question: why is the new directory containing all the cluster-algebra related stuff called cluster_algebra_quiver rather than cluster_algebra?

since it contains cluster_seed.py and quiver.py, I thought that it might be good to have both in the folder name. But I am fine with cluster_algebra as well...

comment:59 in reply to: ↑ 56 Changed 5 years ago by stumpc5

Replying to hthomas:

Hello, Christian and Nicolas--

The Cartan matrix for finite type B is the same, whether we go via CartanType or QuiverMutationType (good, and not a surprise).

We only followed the Sage convention, so it doesn't say much that both are the same.

comment:60 in reply to: ↑ 58 ; follow-up: Changed 5 years ago by hthomas

Replying to stumpc5:

Replying to hthomas:

Very minor question: why is the new directory containing all the cluster-algebra related stuff called cluster_algebra_quiver rather than cluster_algebra?

since it contains cluster_seed.py and quiver.py, I thought that it might be good to have both in the folder name. But I am fine with cluster_algebra as well...

Sorry, I hadn't yet looked at quiver.py, and wasn't aware of all that was in there. At a first glance, I'm very happy to see that this exists!

I don't know what the rules/principles for organizing sage code are, but it seems to me that the quiver stuff will be of interest well beyond its cluster algebra applications. Should it be less hidden? Maybe quiver.py should be a file in the combinat directory? Or maybe it should be somewhere in with more algebra-related code?

There was some discussion of QPA at Sage Days 38, and I don't think people were aware of the quiver code in your patches, so the possibility that it could escape notice is of some concern. (Maybe less concern once it gets really into Sage, though, I guess.)

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

Sorry, I hadn't yet looked at quiver.py, and wasn't aware of all that was in there. At a first glance, I'm very happy to see that this exists!

I don't know what the rules/principles for organizing sage code are, but it seems to me that the quiver stuff will be of interest well beyond its cluster algebra applications. Should it be less hidden? Maybe quiver.py should be a file in the combinat directory? Or maybe it should be somewhere in with more algebra-related code?

There was some discussion of QPA at Sage Days 38, and I don't think people were aware of the quiver code in your patches, so the possibility that it could escape notice is of some concern. (Maybe less concern once it gets really into Sage, though, I guess.)

We had a discussion on quivers in cluster algebra and in representation theory, see #12630. We decided in the end, that we will rename our quivers for clusters to combinatorial_quivers (since they can be mutated and stuff, which doesn't make any sense for quivers as input for path algebras).

comment:62 in reply to: ↑ 61 Changed 5 years ago by hthomas

Replying to stumpc5:

We had a discussion on quivers in cluster algebra and in representation theory, see #12630. We decided in the end, that we will rename our quivers for clusters to combinatorial_quivers (since they can be mutated and stuff, which doesn't make any sense for quivers as input for path algebras).

Thanks for pointing me to #12630.

I'm not really seeing the logic of what is being done here (well, really I mean #10538) vs what is being done there vs what is now being done both places. I realize that you and Jim Stark didn't sit down and plan this out a year ago! But to me, at this point, it doesn't make sense to have two different sets of code in Sage implementing the same things. There is some functionality which you have which he doesn't (eg, reflection functors), as well as functionality which he has and you don't, and I think the right thing to do is to think about merging.

This suggestion isn't intended to impact any of the cluster algebra code, which has a different orientation and solves a different set of problems (in particular, I get it that in cluster algebras, it's most natural to have mutable quivers, while Jim wants immutable quivers).

comment:63 follow-up: Changed 5 years ago by hthomas

In cluster algebras 2, beginning of section 6, Fomin and Zelevinsky point out that they are using the transpose of the Bourbaki convention for associating a root system to a Cartan matrix (and that the convention they are using is consistent with Kac). So it looks like the choice of convention that you & Nicolas were following in consistent with F&Z, and Kac, and transposed compared to Bourbaki.

I think it would be good to add a note to this effect to the documentation for the cartan_matrix of a CartanType, and probably also for the cartan_matrix of a QuiverMutationType.

comment:64 in reply to: ↑ 63 Changed 5 years ago by nthiery

Replying to hthomas:

In cluster algebras 2, beginning of section 6, Fomin and Zelevinsky point out that they are using the transpose of the Bourbaki convention for associating a root system to a Cartan matrix (and that the convention they are using is consistent with Kac). So it looks like the choice of convention that you & Nicolas were following in consistent with F&Z, and Kac, and transposed compared to Bourbaki.

Hmm, I am surprised: the doc states that this is Bourbaki's convention, and Dan is the one that entered the data, and he is careful with this kind of things. I think we discussed those things on the Sage-Combinat mailing list a couple years ago; you may want to check this out. One think to double check also is whether the dynkin diagrams are consistent.

Cheers,

Nicolas

comment:65 Changed 5 years ago by hthomas

Hi Nicolas and Christian--

The only relevant reference I found to Bourbaki is in root_system/cartan_type.py where it says that the *numbering* of roots follows Bourbaki. What I am saying is not contrary to this.

In sage-combinat-devel, in June of 2008, in the thread "tensor product of crystal code", I found Dan Bump saying the following:

It says so in the documentation:

The edge multiplicities are encoded as edge labels. This uses the convention in Kac / Fulton Harris Representation theory wikipedia http://en.wikipedia.org/wiki/Dynkin_diagram, that is for i != j:

j -k-> i <==> a_ij = -k <==> -scalar(coroot[i],root[j]) = k <==> multiple arrows point from the longer root to the shorter one

That is the standard convention but in fact what is implemented is

j -k-> i <==> a_ji = -k

See this line in cartan_matrix.py():

cmf = lambda i,j: 2 if i == j else -f(j,i)

Since Kac and Bourbaki are contradictory (according to F&Z), everything is consistent.

Dan seems to be unaware that the Kac convention which he calls "standard" is contrary to Bourbaki, but this would be an easy thing not to be aware of.

I still think it would be useful to say explicitly in the cartan_matrix methods of CartanType and QuiverMutationType that the cartan_matrix is according to Kac/Fulton-Harris and contrary to Bourbaki.

I did also double-check the Dynkin diagrams, and they are correct consistent with this.

And just to clarify, that means that all the finite type QuiverMutationTypes are consistent with what was already in Sage and consistent with Fomin and Zelevinsky (which is not surprising -- otherwise I'm sure it would have been picked up on already).

cheers,

Hugh

comment:66 follow-up: Changed 5 years ago by hthomas

Hi!

I have some further mathematical corrections to QuiverMutationType.

Affine C is CC (not BC).

D,n,2 is BB,n-1,1 (not CC,n,1).

G,2,1 and G,2,-1 are reversed from what they should be. (When this is fixed it will also fix D,4,3)

For consistency, I think:

  • C,n,-1 should return the dual of C,n,1, i.e., BB (right now it gives an error).
  • B,n,-1 should return the dual of B,n,1, i.e., CD (ditto)

I think I have now checked the data for affine types.

I will go ahead and make these changes but I wanted to record them here in case anyone had any corrections to my corrections.

cheers,

Hugh

comment:67 follow-up: Changed 5 years ago by hthomas

Also, FYI, the diagrams for A(2)n at http://en.wikipedia.org/wiki/Dynkin_diagram#Affine_Dynkin_diagrams are wrong (the even and odd subscripts are swapped). I've put a note on the talk page. (I mention this because I -- and maybe others of us -- have been using this as a reference.)

comment:68 in reply to: ↑ 67 Changed 5 years ago by nthiery

Replying to hthomas:

Also, FYI, the diagrams for A(2)n at http://en.wikipedia.org/wiki/Dynkin_diagram#Affine_Dynkin_diagrams are wrong (the even and odd subscripts are swapped). I've put a note on the talk page. (I mention this because I -- and maybe others of us -- have been using this as a reference.)

Please bring this discussion to sage-combinat-devel, as this had, in principle, been done carefuly (but w.r.t which convention, I don't remember).

comment:69 in reply to: ↑ 66 Changed 5 years ago by hthomas

Replying to hthomas:

I have now made the changes I suggested in comment 66.

comment:70 Changed 5 years ago by hthomas

I've made a few more minor changes, mostly to the documentation, and also catching that (T,(2,4,4)) = (E,7,1). I hope to be done #10527 soon!

I think the property "affine" should only be implemented for irreducible quiver mutation types. It's not clear what it means for an reducible quiver mutation type to be affine. (A product of two affines is, in my view, less affine than a product of an affine and a Dynkin is, since in the latter case, but not the former, the radical of the symmetric form is one-dimensional.)

I feel similarly about "elliptic" (though I have less mathematical knowledge to back up my gut feeling that the product of two elliptics isn't very elliptic).

Thoughts?

comment:71 Changed 5 years ago by chapoton

Hello,

in order to make the bot turn green,

  • you could add the ticket number (#10527) to the (first line of the) description of every patch,
  • and remove all trailing whitespaces in the patches.

comment:72 Changed 5 years ago by hthomas

Frédéric, we are keeping the current version of the patches associated to this ticket on the combinat server. The patches currently attached to the ticket are not current. (However, on a different ticket, I was wondering why the patchbot was unhappy, and I think you may have explained it, so thanks!)

*

I have made the changes I suggested in comment 70.

comment:73 follow-up: Changed 5 years ago by hthomas

Hi!

I just noticed that class_size() did not return the correct answer when applied to a reducible type in which some irreducible type appears more than once (because in this case, you cannot simply multiply the sizes of the classes). I have fixed this.

cheers,

Hugh

comment:74 in reply to: ↑ 73 Changed 5 years ago by stumpc5

Replying to hthomas:

Hi!

I just noticed that class_size() did not return the correct answer when applied to a reducible type in which some irreducible type appears more than once (because in this case, you cannot simply multiply the sizes of the classes). I have fixed this.

Thanks -- I didn't notice when I was writing the code...

comment:75 Changed 5 years ago by stumpc5

  • Dependencies #10349 deleted

comment:76 follow-up: Changed 5 years ago by gmoose05

Small patch added to clean-up a few documentation issues:

*Examples of Kac' notation,

*Ensuring warnings about class_size() prints,

*References for class_size(),

and * updating _mutation_type_error().

comment:77 follow-up: Changed 5 years ago by hthomas

Hi Gregg--

Thanks for your changes, which are all good.

The patchbot will be happier if you remove the trailing space at the end of the line "Conjectural formulas for several other non-simply-laced affine types are implemented."

Is there any reason not to put your patch on the combinat server as well? If you are uncomfortable doing that, I can take care of it. I checked, and it doesn't introduce any issues with the subsequent patches.

cheers,

Hugh

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

Replying to gmoose05:

Small patch added to clean-up a few documentation issues: *Examples of Kac' notation,

Are you sure the double colons in lines 432 and 457 print nicely in the documentation? If I remember right, these should be single colons here.

*Ensuring warnings about class_size() prints,

What about line 1457?

Everything else looks good to me!

comment:79 Changed 5 years ago by hthomas

Hi Gregg, hi Christian!

Another technicality. To produce a patch for posting on trac, you are supposed to do something like the following:

hg qrefresh -m "10527: minor fixes to documentation"
hg export tip > ~/trac_10527-quiver-mutation-type-gm.patch

This creates a patch from your current tip in your home directory which has more information about where it came from and what it's doing. The difference is visible in the first few lines of the patch (e.g., if you click on the file attached to the ticket). Christian's looks like it's suposed to, as far as I understand. Mine (which Christian uploaded) looks pretty wierd to me -- Christian, are you sure it's okay? Gregg's is missing information, maybe because it was just copied from the .hg/patches directory.

This is stuff about which I'm definitely not an expert -- I just try to make my patch look like other ones do. I have had a patch rejected by the release manager because I hadn't done these steps properly, though, which delays everything and wastes his time, so I'm not just being picky for nothing.

(It's fine not to worry about this until we're certain everything else is finalized.)

cheers,

Hugh

Last edited 5 years ago by hthomas (previous) (diff)

comment:80 in reply to: ↑ 78 Changed 5 years ago by gmoose05

Replying to stumpc5:

Replying to gmoose05:

Small patch added to clean-up a few documentation issues: *Examples of Kac' notation,

Are you sure the double colons in lines 432 and 457 print nicely in the documentation? If I remember right, these should be single colons here.

I will look more at the documentation and get back to you. Perhaps these should be changed backed, I thought the single colons were simply typos so might have changed these in error.

*Ensuring warnings about class_size() prints,

What about line 1457?

Thanks for the heads up, although Line 1457 is an example rather than part of the source code. Hence why there is no "print" command there.

Everything else looks good to me!

comment:81 in reply to: ↑ 77 Changed 5 years ago by gmoose05

Replying to hthomas:

Hi Gregg--

Thanks for your changes, which are all good.

The patchbot will be happier if you remove the trailing space at the end of the line "Conjectural formulas for several other non-simply-laced affine types are implemented."

Done.

Is there any reason not to put your patch on the combinat server as well? If you are uncomfortable doing that, I can take care of it. I checked, and it doesn't introduce any issues with the subsequent patches.

I should be able to move it to the combinat queue this afternoon. I will make some of these other tiny fixes first and post consistent versions both places.

cheers,

Hugh

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

Hi,

I updated Gregg's patch (and deleted the one he accidentally added), and also set the double colons right (Gregg,please confirm). Unfortunately, I somehow cannot build the documentation (and Gregg cannot either). Hugh, could you please recheck? Also, the patchbot is down.

I hope everything is fine now, let's see what the patchbot says and also how we get the docbuild running again.

Finally, I didn't do any changes on sage-combinat, please let us NOT do things for this ticket on combinat since it always takes forever to rebuild the queue and also the documentation, and other patches in there spoil the doc as well. So only the patches here on trac are up-to-date!!!

comment:83 in reply to: ↑ 82 Changed 5 years ago by stumpc5

Replying to stumpc5:

I updated Gregg's patch (and deleted the one he accidentally added), and also set the double colons right (Gregg,please confirm). Unfortunately, I somehow cannot build the documentation (and Gregg cannot either). Hugh, could you please recheck?

Okay, it worked on my computer as follows (I am not saying this is the way to go, but it happened to work): after building a new branch, I pushed the three patches above, and then ran

sage -docbuild -i reference html

This failed with some ascii error somewhere but running

sage -docbuild reference html

afterwards then rebuilt everything (which was needed in order to get the folder cluster_algebra_quiver recognized by the documentation builder.

The doc looks good, in particular the colons in various places.

Best, Christian

comment:84 Changed 5 years ago by hthomas

Sorry for the delay. I still had a bit of mathematical review to do. I corrected one significant error, in the calculation of duals for non-simply-laced elliptics (though I can't claim a lot of credit for having done so, since it was an error I had introduced myself at an earlierstage).

Types B,2,1 and B,2,-1 are now aliases for CC,2,1 (=C,2,1) and BB,2,1 (=C,2,-1). (I.e., the affine types associated to B,2). Formerly this produced an error.

I fiddled a little bit with phrasing in the documentation.

I made the titles referred to in the class_size method agree with the articles being referred to.

There is a mention in the documentation of "class_size" that the formulas for affine B and affine C have been proved, but the proof is unpublished. In the code, though, it says that the formulas are not proved. This should be corrected to be consistent. I didn't do it myself because I wasn't completely clear which formulas it was being claimed had been proved: affine C and its dual (which is not affine B) or affine C and affine B (and their duals) or just affine C and affine B.

I think I see that the elements of a class and its dual class are naturally in bijective correspondence in acyclic types, but it's not obvious to me outside that setting (though that's all that matters here).

comment:85 Changed 5 years ago by dimpase

Please specify patches to be applied. Otherwise the patchbot (and the release manager) may be confused... As well, the dependencies on the other (not yet merged) tickets should be listed in Dependencies: field.

comment:86 follow-up: Changed 5 years ago by hthomas

Hi Dmitrii--

The patches are all to be applied, in order. The patchbot is doing the right thing. Patch 2 of the 4 ("-ht.patch") has some wierd mercurial business at its front end, which the authors are aware of and will fix.

There are no non-merged dependencies. The general plan for the cluster algebra implementation is at #10298.

cheers,

Hugh

comment:87 Changed 5 years ago by hthomas

  • Description modified (diff)

comment:88 in reply to: ↑ 86 ; follow-up: Changed 5 years ago by dimpase

Replying to hthomas:

The patches are all to be applied, in order. The patchbot is doing the right thing. Patch 2 of the 4 ("-ht.patch") has some wierd mercurial business at its front end, which the authors are aware of and will fix.

one can just edit the patch file, so that it top part looks like

# HG changeset patch
# User Christian Stump <christian.stump at gmail.com>
# Date 1336581225 14400
# Node ID 0251afca92eb1a64a7b848d0269664fc7db05529
# Parent 2f8b17ada7858731154eee8e7d84be8690485627
trac 10527-quiver_mutation_type-ht.patch

diff --git a/doc/en/reference/combinat/index.rst b/doc/en/reference/combinat/index.rst

It'd be easiest if the submitter (Christian S.) does this, then he can just replace the uploaded file with the edited one.

Looks OK, otherwise.

Dima

comment:89 in reply to: ↑ 88 ; follow-up: Changed 5 years ago by hthomas

Replying to dimpase:

Looks OK, otherwise.

Thanks!

comment:90 Changed 5 years ago by dimpase

I also noticed that in several places it should be EXAMPLES:: rather than EXAMPLES:. This will make sphinx happy. Did you actually try building the documentation in question?

Oops, sorry, my fault. It all seems to work just fine. I should learn more about sphinx, it seems...

Last edited 5 years ago by dimpase (previous) (diff)

comment:91 in reply to: ↑ 89 Changed 5 years ago by stumpc5

Replying to hthomas:

Replying to dimpase:

Looks OK, otherwise.

Thanks!

I updated the headers, I hope they are all fine now! Hugh, are you now willing to set the positive review?

comment:92 Changed 5 years ago by stumpc5

I now lazily import quiver mutation types in order to not slow down the startup time.

comment:93 Changed 5 years ago by stumpc5

Can anyone tell me what to think about the patchbot doctest failures? I don't know how to solve them.

Thanks, Christian

comment:94 Changed 5 years ago by hthomas

Hi--

#12496 is another ticket of mine where the patchbot is now complaining, giving word-for-word the same errors as the sagepad.org patchbot on this ticket. So I think the problem is not with our tickets. But I am surprised no one else is reporting it on sage-devel.

cheers,

Hugh

comment:95 Changed 5 years ago by gmoose05

Hi all,

To be more precise, the patchbot appears to come up with the doctest error

 sage -t  -force_lib devel/sage-10527/sage/misc/sagedoc.py ********************************************************************** File "/home/patchbot/Sage/sage-5.3.rc1/devel/sage-10527/sage/misc/sagedoc.py", line 22:

  sage: for line in open(docfilename):
    if "#sage.symbolic.expression.Expression.N" in line:
      print line

Exception raised:

  Traceback (most recent call last):
    File "/home/patchbot/Sage/sage-5.3.rc1/local/bin/ncadoctest.py", line 1231, in run_one_test
      self.run_one_example(test, example, filename, compileflags)
    File "/home/patchbot/Sage/sage-5.3.rc1/local/bin/sagedoctest.py", line 38, in run_one_example
      OrigDocTestRunner .run_one_example(self, test, example, filename, compileflags)
    File "/home/patchbot/Sage/sage-5.3.rc1/local/bin/ncadoctest.py", line 1172, in run_one_example
      compileflags, 1) in test.globs
    File "<doctest __ main__ .example_0[3] >", line 1, in <module>
      for line in open(docfilename):###line 22:
  sage: for line in open(docfilename): IOError: [Errno 2] No such file or directory: '/home/patchbot/Sage/sage-5.3.rc1/devel/sage/doc/output/html/en/reference/sage/symbolic/expression.html'

********************************************************************** File "/home/patchbot/Sage/sage-5.3.rc1/devel/sage-10527/sage/misc/sagedoc.py", line 1221:

  sage: browse_sage_doc._open("reference", testing=True)[0]   # indirect doctest

Exception raised:

  Traceback (most recent call last):
    File "/home/patchbot/Sage/sage-5.3.rc1/local/bin/ncadoctest.py", line 1231, in run_one_test
      self.run_one_example(test, example, filename, compileflags)
    File "/home/patchbot/Sage/sage-5.3.rc1/local/bin/sagedoctest.py", line 38, in run_one_example
      OrigDocTestRunner .run_one_example(self, test, example, filename, compileflags)
    File "/home/patchbot/Sage/sage-5.3.rc1/local/bin/ncadoctest.py", line 1172, in run_one_example
      compileflags, 1) in test.globs
    File "<doctest __ main__ .example_19[2] >", line 1, in <module>
      browse_sage_doc._open("reference", testing=True)[Integer(0)]  # indirect doctest###line 1221:
  sage: browse_sage_doc._open("reference", testing=True)[0]   # indirect doctest
    File "/home/patchbot/Sage/sage-5.3.rc1/local/lib/python/site-packages/sage/misc/sagedoc.py", line 1392, in _open
      with 'sage -docbuild %s html --jsmath' and try again.""" %(name, name)
  OSError: The document 'reference' does not exist.  Please build it with 'sage -docbuild reference html --jsmath' and try again.

********************************************************************** File "/home/patchbot/Sage/sage-5.3.rc1/devel/sage-10527/sage/misc/sagedoc.py", line 1383:

  sage: browse_sage_doc._open("reference", testing=True)[0]

Exception raised:

  Traceback (most recent call last):
    File "/home/patchbot/Sage/sage-5.3.rc1/local/bin/ncadoctest.py", line 1231, in run_one_test
      self.run_one_example(test, example, filename, compileflags)
    File "/home/patchbot/Sage/sage-5.3.rc1/local/bin/sagedoctest.py", line 38, in run_one_example
      OrigDocTestRunner .run_one_example(self, test, example, filename, compileflags)
    File "/home/patchbot/Sage/sage-5.3.rc1/local/bin/ncadoctest.py", line 1172, in run_one_example
      compileflags, 1) in test.globs
    File "<doctest __ main__ .example_23[2] >", line 1, in <module>
      browse_sage_doc._open("reference", testing=True)[Integer(0)]###line 1383:
  sage: browse_sage_doc._open("reference", testing=True)[0]
    File "/home/patchbot/Sage/sage-5.3.rc1/local/lib/python/site-packages/sage/misc/sagedoc.py", line 1392, in _open
      with 'sage -docbuild %s html --jsmath' and try again.""" %(name, name)
  OSError: The document 'reference' does not exist.  Please build it with 'sage -docbuild reference html --jsmath' and try again.

********************************************************************** File "/home/patchbot/Sage/sage-5.3.rc1/devel/sage-10527/sage/misc/sagedoc.py", line 1385:

  sage: browse_sage_doc._open("tutorial", testing=True)[1]

Exception raised:

  Traceback (most recent call last):
    File "/home/patchbot/Sage/sage-5.3.rc1/local/bin/ncadoctest.py", line 1231, in run_one_test
      self.run_one_example(test, example, filename, compileflags)
    File "/home/patchbot/Sage/sage-5.3.rc1/local/bin/sagedoctest.py", line 38, in run_one_example
      OrigDocTestRunner .run_one_example(self, test, example, filename, compileflags)
    File "/home/patchbot/Sage/sage-5.3.rc1/local/bin/ncadoctest.py", line 1172, in run_one_example
      compileflags, 1) in test.globs
    File "<doctest __ main__ .example_23[3] >", line 1, in <module>
      browse_sage_doc._open("tutorial", testing=True)[Integer(1)]###line 1385:
  sage: browse_sage_doc._open("tutorial", testing=True)[1]
    File "/home/patchbot/Sage/sage-5.3.rc1/local/lib/python/site-packages/sage/misc/sagedoc.py", line 1392, in _open
      with 'sage -docbuild %s html --jsmath' and try again.""" %(name, name)
  OSError: The document 'tutorial' does not exist.  Please build it with 'sage -docbuild tutorial html --jsmath' and try again.

********************************************************************** 3 items had failures:

  1 of   5 in __ main__ .example_0 1 of   5 in __ main__ .example_19 2 of   5 in __ main__ .example_23

***Test Failed*** 4 failures.

However, I just downloaded 5.3.rc1, applied the five patches, and ran

sage -bt sage/misc

which returned:

all All tests passed!

including:

sage -t  "devel/sage-testing/sage/misc/sagedoc.py"

  [24.5 s]

Total time for all tests: 475.7 seconds

Since we cannot measure whether other tickets (that depend on 10527) are passing tests with the patchbot until this is resolved, has anyone encountered this error elsewhere? Or know how to resolve this? It appears to be a problem with the patchbot itself, rather than the patches.

Nicolas, should we be posting this on devel google-groups?

comment:96 follow-up: Changed 5 years ago by hthomas

Hi!

I posted a thread on sage-devel about the sagepad.org patchbot -- it seems to be having a problem, as it is failing every patch.

Volker's patchbot, though, Fedora/17/x86_64/3.5.2-3.fc17.x86_64/volker-desktop.stp.dias.ie, seems to be being confused by the lazy import. (Search for "lazy_import" in the patchbot log file.)

I don't know what the right thing to do about that is. The easy option would be to change the import back to normal, and leave it to someone else who knows lazy imports (and maybe also the patchbot) better, to fix it. Since there is no special reason that the cluster stuff has to be lazily imported, this seems pretty reasonable to me.

cheers,

Hugh

comment:97 in reply to: ↑ 96 ; follow-ups: Changed 5 years ago by nthiery

Replying to hthomas:

I posted a thread on sage-devel about the sagepad.org patchbot -- it seems to be having a problem, as it is failing every patch.

Volker's patchbot, though, Fedora/17/x86_64/3.5.2-3.fc17.x86_64/volker-desktop.stp.dias.ie, seems to be being confused by the lazy import. (Search for "lazy_import" in the patchbot log file.)

I don't know what the right thing to do about that is. The easy option would be to change the import back to normal, and leave it to someone else who knows lazy imports (and maybe also the patchbot) better, to fix it. Since there is no special reason that the cluster stuff has to be lazily imported, this seems pretty reasonable to me.

I looked a bit, and the place it is failing at is when trying to figure out which names would be imported by the "*" (sage.misc.lazy_import.get_star_imports). And indeed this piece is a bit tricky and therefore likely to be brittle.

I would suggest to avoid using the "*" feature of lazy_import by either:

(1) Puting explicitely in combinat.all the list of things to be lazily imported:

lazy_import("sage.combinat.cluster_algebra_quiver.all", "QuiverMutationType?,...", overwrite=False)

(2) Replace the "lazy import *" in combinat.all by a usual import *,

and use instead lazy imports on each piece in sage.combinat.cluster_algebra_quiver.all

Solution 2 has the advantage of not having to touch again combinat.all in later patches, knowing that this file is very often changed and therefore a source of potential conflicts.

By the way: unless there is a strong reason not to, I would not leave "overwrite" to its default value.

Cheers,

Nicolas

comment:98 in reply to: ↑ 97 Changed 5 years ago by stumpc5

(2) Replace the "lazy import *" in combinat.all by a usual import *,

and use instead lazy imports on each piece in sage.combinat.cluster_algebra_quiver.all

By the way: unless there is a strong reason not to, I would not leave "overwrite" to its default value.

Thanks Nicolas for the explanation, I will take care of as soon as I finished compiling 5.3.rc1.

Christian

comment:99 in reply to: ↑ 97 Changed 5 years ago by nthiery

Replying to nthiery:

By the way: unless there is a strong reason not to, I would not leave "overwrite" to its default value.

I of course meant: "I would leave overwrite to its default value".

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

Since we all agreed that the patch is ready, I folded all patches, and fixed the lazy import as Nicolas suggested (thanks again for investigating the problem!).

Christian

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

Replying to stumpc5:

Since we all agreed that the patch is ready, I folded all patches, and fixed the lazy import as Nicolas suggested (thanks again for investigating the problem!).

I already see that this makes the startup modules plugin unhappy. Is that still fine, or do we have to find a different solution (I import cluster_algebra_quiver.all in combinat.all, and then lazily import QuiverMutationType? inside cluster_algebra_quiver.all)?

comment:102 in reply to: ↑ 101 Changed 5 years ago by stumpc5

Replying to stumpc5:

Replying to stumpc5: I already see that this makes the startup modules plugin unhappy.

One patchbot doesn't like the startup but does like the coverage, and it's the other way round with the other patchbot...

comment:103 Changed 5 years ago by nthiery

Are the following patchbot reports wrong?

checking consistency... /mnt/storage2TB/patchbot/Sage/sage-5.3.rc1/devel/sage/doc/en/reference/sage/combinat/cluster_algebra_quiver/quiver_mutation_type.rst:: WARNING: document isn't included in any toctree

Missing doctests  combinat/cluster_algebra_quiver/quiver_mutation_type.py 19 / 31 = 61%

comment:104 follow-up: Changed 5 years ago by hthomas

@Nicolas -- I downloaded the patch and ran sage -coverage on quiver_mutation_type.py; it says we have doctests for 30/30. (I also looked myself, and don't see any problems.) The documentation for quiver_mutation_type.py also seems to be where it belongs in the documentation. I don't know what the patchbot is smoking.

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

Replying to hthomas:

@Nicolas -- I downloaded the patch and ran sage -coverage on quiver_mutation_type.py; it says we have doctests for 30/30. (I also looked myself, and don't see any problems.) The documentation for quiver_mutation_type.py also seems to be where it belongs in the documentation. I don't know what the patchbot is smoking.

Same here, I also get 30/30 in quiver_mutation_type.py with 5.3.rc1 with only this patch applied.

comment:106 in reply to: ↑ 105 Changed 5 years ago by nthiery

Replying to stumpc5:

Replying to hthomas:

@Nicolas -- I downloaded the patch and ran sage -coverage on quiver_mutation_type.py; it says we have doctests for 30/30. (I also looked myself, and don't see any problems.) The documentation for quiver_mutation_type.py also seems to be where it belongs in the documentation. I don't know what the patchbot is smoking.

Same here, I also get 30/30 in quiver_mutation_type.py with 5.3.rc1 with only this patch applied.

Hmm. Then triple check the order in which the patchbot applied the patches, and report to the patchbot maintainer!

comment:107 Changed 5 years ago by hthomas

I am pretty confident the patches are being applied in the right order! (Since there is now only one.)

comment:108 Changed 5 years ago by hthomas

From the log file, it looks like the patchbot isn't trying to patch doc/en/reference/combinat/index.rst. That would be consistent with the complaint about the docs not winding up in any toc tree. That part of the patch comes right after the hg-related cruft. Christian, perhaps you could remove the cruft?

comment:109 Changed 5 years ago by hthomas

I tried removing the cruft and commenting out the commit message.

Last edited 5 years ago by stumpc5 (previous) (diff)

comment:110 follow-up: Changed 5 years ago by hthomas

Drat, I don't seem to be able to convince the patchbot not to try applying the first patch. Sorry Christian, I guess you're our only hope now :).

comment:111 in reply to: ↑ 110 Changed 5 years ago by stumpc5

Replying to hthomas:

Drat, I don't seem to be able to convince the patchbot not to try applying the first patch. Sorry Christian, I guess you're our only hope now :).

Sorry for being slow -- let's see what happens now...

comment:112 Changed 5 years ago by gmoose05

  • Description modified (diff)

comment:113 follow-up: Changed 5 years ago by hthomas

Yay! So far as I can tell, the only complaint the patchbot is making is that we're adding new modules, but since we're importing them lazily, it's not really a problem.

I had never reviewed Gregg's final patch. I've now looked at it and I'm afraid I have some minor issues with it. (And testing it has led to detecting some other non-optimal behaviour.)

Gregg, you said you wanted to fix QuiverMutationType('C',1,1) giving a cryptic message, but in fact it still does (it still says "'CC',1,1" is not valid).

I see you changed the error message to reflect the fact that in elliptic types, twists can have 2's in them, which is better than what was there before. But they can also have 3's in them (elliptic G's).

In fact, I suggest removing the last four lines of the error message, including the elliptics, and replacing them by "For correct syntax in other types, please consult the documentation." As it stands, it looks like it's purporting to be some kind of complete list, but in fact not every type is well-described (e.g., GR, which takes a single additional parameter which is a tuple/list blah, blah). In general, I don't think the goal of an error message should be to replace the documentation.

I still think it's pointless to include AE. If you want it, it definitely shouldn't have a twist of 3. And I think the documentation should explain the input format.

I'm not sure why we're allowing QuiverMutationType('GR',(n,k)) as well as (k,n). The Grassmannian of 3-planes in 2-space is just meaningless, and (in my view) it should return an error. I could be argued out of this.

QuiverMutationType('D',3,2) works, but QuiverMutationType('A',3,2) doesn't. But A_3 and D_3 are the same. I think both should yield what D,3,2 does now (namely, C,2,-1). There has to be some notation for it, and it's pretty peculiar to require people to think of the root system as derived from D,3 rather than A,3. (The reason this makes some sense is that it fits the pattern of D,n,2, not the pattern of A,2n+1,2, but I think that's okay.)

comment:114 in reply to: ↑ 113 ; follow-up: Changed 5 years ago by gmoose05

  • Cc robertwb added

I just uploaded a new patch which takes care of the issues that Hugh raised. Thanks for that. I also made one or two other changes that will be useful for patches 10538 going forward. Please see my comments below.

Replying to hthomas:

Yay! So far as I can tell, the only complaint the patchbot is making is that we're adding new modules, but since we're importing them lazily, it's not really a problem.

Yay! Hopefully the patchbot will again be happy after it processes the new patch. Yes, this seems related to https://groups.google.com/forum/?fromgroups=#!searchin/sage-devel/patchbot$20plugin$20start-up$20modules/sage-devel/6LByB07Ks9c/gU1TCecUwwoJ

and yes, it is true that we import (although lazily) modules. So I agree that this plug-in is not an issue. To double-check, I just CC'ed Robert Bradshaw (who had also mentioned the lazy_import to us for #10538).

I had never reviewed Gregg's final patch. I've now looked at it and I'm afraid I have some minor issues with it. (And testing it has led to detecting some other non-optimal behaviour.)

Gregg, you said you wanted to fix QuiverMutationType('C',1,1) giving a cryptic message, but in fact it still does (it still says "'CC',1,1" is not valid).

Yes, this is fixed now. Thanks for catching that.

I see you changed the error message to reflect the fact that in elliptic types, twists can have 2's in them, which is better than what was there before. But they can also have 3's in them (elliptic G's).

In fact, I suggest removing the last four lines of the error message, including the elliptics, and replacing them by "For correct syntax in other types, please consult the documentation." As it stands, it looks like it's purporting to be some kind of complete list, but in fact not every type is well-described (e.g., GR, which takes a single additional parameter which is a tuple/list blah, blah). In general, I don't think the goal of an error message should be to replace the documentation.

Yes, you are correct I slightly changed the phrasing for Elliptic, and then followed your advice for the last three lines. Let me know if this looks okay.

I still think it's pointless to include AE. If you want it, it definitely shouldn't have a twist of 3. And I think the documentation should explain the input format.

I ensured that AE doesn't take a twist argument, updated the example, and added a comment to the documentation about the input format. I still vote to keep it, but don't feel too strongly if you and Christian want to get rid of it.

I'm not sure why we're allowing QuiverMutationType('GR',(n,k)) as well as (k,n). The Grassmannian of 3-planes in 2-space is just meaningless, and (in my view) it should return an error. I could be argued out of this.

I forget now why we allowed switching the order of k and n. Christian, do you remember? Anyway, Hugh I agree with you, and made the change. QuiverMutationType('GR',(5,2)) for instance now gives an error. But we still have QuiverMutationType('GR',(3,5)) = QuiverMutationType('GR',(2,5)).

QuiverMutationType('D',3,2) works, but QuiverMutationType('A',3,2) doesn't. But A_3 and D_3 are the same. I think both should yield what D,3,2 does now (namely, C,2,-1). There has to be some notation for it, and it's pretty peculiar to require people to think of the root system as derived from D,3 rather than A,3. (The reason this makes some sense is that it fits the pattern of D,n,2, not the pattern of A,2n+1,2, but I think that's okay.)

This was from following Kac's affine notation, which didn't allow A_3(2).

However, coercing to ('D',3,2) might be a better choice than an error message here. Unfortunately, ('A',3,2) doesn't resemble ('A',2k+1,2) in this case, but perhaps this is okay. Christian, feel free to let us know if you have a different preference. By the way, I just ran

sage: CartanType(['A',3,2])
['B', 2, 1]^*
sage: CartanType(['D',3,2])
['C', 2, 1]^*
sage: CartanType(['A',4,2])
['BC', 2, 2]
sage: CartanType(['A',5,2])
['B', 3, 1]^*
sage: CartanType(['A',7,2])
['B', 4, 1]^*
sage: CartanType(['D',4,2])
['C', 3, 1]^*
sage: CartanType(['D',5,2])
['C', 4, 1]^*

as a sanity check, and it actually *does differ* from this choice. Should we make

sage: QuiverMutationType(['D',3,2])
['BB',2,1]
sage: QuiverMutationType(['A',3,2])
['CD',2,1]

instead?

comment:115 follow-up: Changed 5 years ago by gmoose05

Also, in my most recent patch, I also did the following:

  • I separated out the cases ['F', 4, [1,2]] and ['F', 4, [2,1]], as well as

['G', 4, [1,3]] and ['G', 4, [3,1]] in the class !QuiverMutationType_Irreducible.

This is in preparation for changes I will be making to the ClusterQuiver class. These specific changes used to be in the ticket #10538, see item (8a) of http://trac.sagemath.org/sage_trac/ticket/10538#comment:18, but since I was changing this patch anyway, this ticket was a more natural place to have these changes.

Once we have finalized this ticket, I will update #10538 accordingly to fix the impending patch conflict.

  • I also added the coercions ['F',4,[2,1]] -> ['F',4,[1,2]] and ['G',2,[3,1]] -> ['G',2,[1,3]] up front in the Mutation Type Casting list to make this separation clearer. The point is that we can think of these four cases each as different quivers, even though the two quivers of type F listed here are mutation-equivalent, and same for the elliptic G quivers listed here.

Similarly, the values ['GR', [2,n]] and ['TR',1], ['TR',2] are now allowed as inputs to !QuiverMutationType_Irreducible

  • See also the discussion in comments 23-25 of #10538. I also now allow the user to type in
sage: QuiverMutationType(['A',[0,7],1])
['D',7]

This will later allow the user to construct the cyclic quiver on n-vertices by specifying 0 clockwise arrows in a quiver of "type affine A". Note that such a quiver is actually of finite D type, not affine A.

comment:116 Changed 5 years ago by robertwb

Lazily importing * is preferred, as it avoids stat'ing a new directory and loading two new files (init.py and all.py). All this filesystem activity seems to be one of the primary reasons for slow Sage startup time (perhaps even more so than the execution of the .py files themselves) and if we're going to try to fix that it's important to be careful about what continues to be added.

That being said, I have know idea what's up with volker-desktop.stp.dias.ie . It's as if the file existed but was incomplete/empty. Let's see if that continues to be a problem.

comment:117 in reply to: ↑ 114 Changed 5 years ago by hthomas

Replying to gmoose05:

I just uploaded a new patch which takes care of the issues that Hugh raised. Thanks for that.

Thanks!

I also made one or two other changes that will be useful for patches 10538 going forward. Please see my comments below.

It might actually be better to make such changes part of 10538 (I mean, in the cases that they aren't corrections of actual errors in 10527). This way, 10527 gets held up while we debate details which it might make more sense to resolve in the context of where you want to use them. (And perhaps with a more sympathetic referee:).)

On the whole, I don't think it's a good idea to introduce (A,(m,0), 1) as a synonym for D,m. It seems to me that introducing synonyms like this is a process which could go on for ever. There are also wierd corner cases. I think the only reasonable interpretation of A,(1,0),1 is a loop, and of A,(2,0),1 is a 2-cycle. So they just *are not* quiver mutation types. It also seems more confusing than beneficial to me that the quiver which results is not the quiver which you actually named.

Yes, you are correct I slightly changed the phrasing for Elliptic, and then followed your advice for the last three lines. Let me know if this looks okay.

Yes, it's fine.

I ensured that AE doesn't take a twist argument, updated the example, and added a comment to the documentation about the input format. I still vote to keep it, but don't feel too strongly if you and Christian want to get rid of it.

I'm not really arguing against it at this point.

I forget now why we allowed switching the order of k and n. Christian, do you remember? Anyway, Hugh I agree with you, and made the change. QuiverMutationType('GR',(5,2)) for instance now gives an error. But we still have QuiverMutationType('GR',(3,5)) = QuiverMutationType('GR',(2,5)).

I agree that (2,5) and (3,5) should be the same, certainly!

QuiverMutationType('D',3,2) works, but QuiverMutationType('A',3,2) doesn't. But A_3 and D_3 are the same. I think both should yield what D,3,2 does now (namely, C,2,-1). There has to be some notation for it, and it's pretty peculiar to require people to think of the root system as derived from D,3 rather than A,3. (The reason this makes some sense is that it fits the pattern of D,n,2, not the pattern of A,2n+1,2, but I think that's okay.)

This was from following Kac's affine notation, which didn't allow A_3(2).

Interesting! He really writes D_3(2) and not A_3(2)? I'm surprised. To me, it seems in some sense like a feature not a bug that you can ask for A_3(2) and you will be confronted with the one thing it has to be, even if that wasn't what you were expecting.

However, coercing to ('D',3,2) might be a better choice than an error message here. Unfortunately, ('A',3,2) doesn't resemble ('A',2k+1,2) in this case, but perhaps this is okay. Christian, feel free to let us know if you have a different preference. By the way, I just ran

sage: CartanType(['A',3,2])
['B', 2, 1]^*
sage: CartanType(['D',3,2])
['C', 2, 1]^*
sage: CartanType(['A',4,2])
['BC', 2, 2]
sage: CartanType(['A',5,2])
['B', 3, 1]^*
sage: CartanType(['A',7,2])
['B', 4, 1]^*
sage: CartanType(['D',4,2])
['C', 3, 1]^*
sage: CartanType(['D',5,2])
['C', 4, 1]^*

as a sanity check, and it actually *does differ* from this choice.

In fact CartanType('B',2,1) and CartanType('C',2,1) only differ by a reordering of the roots. This is consistent with our current choice, to treat QuiverMutationType('B',2,1) as a synonym for QuiverMutationType('C',2,1). It's also consistent with letting CartanType('A',3,2) and CartanType('D',3,2) be the same (since QuiverMutationType orders roots differently from CartanType anyway).

Should we make

sage: QuiverMutationType(['D',3,2])
['BB',2,1]
sage: QuiverMutationType(['A',3,2])
['CD',2,1]

instead?

No. There is no diagram 'CD',2,1. (In the sense that it isn't drawable.)

comment:118 in reply to: ↑ 115 ; follow-up: Changed 5 years ago by hthomas

Gregg, I'm not sure I understand what you're trying to do with the code for the non-simply-laced elliptics.

It seems to me that the code constructing the digraph for F,4,[2,1] is never called, because that input is automatically recast as F,4,[1,2] before that point in the code is reached.

But if that code were run, I think it would cause problems. As it stands, we have a quiver mutation type called F,4,[1,2]. It has certain attributes, including a digraph. If you could ask for F,4,[2,1] and get a mutation type that was also called F,4,[1,2] but which had a different digraph, I think that would be really confusing (and I think it would screw up the cached methods, too).

It is certainly natural for users to ask for a specific quiver. But if someone really wants to ask for a specific quiver, I don't think the right mechanism for them to do it is for them to ask for a QuiverMutationType by a synonym. They should just ask for the quiver they want, and get that quiver (as an actual quiver, not as a mutation type). And then, depending on the implementation, we either figure out right then or later what the mutation type of the quiver is that they asked for. I think we should think of QuiverMutationType(X) as just giving you a default quiver, on the assumption that the user hasn't really specified a particular quiver.

This is also my real issue with the synonyms you added.

comment:119 in reply to: ↑ 118 Changed 5 years ago by hthomas

Gregg, I've looked at #10538, so now I think I understand what the point of the elliptics code you introduced was (i.e., it wasn't actually supposed to change the QuiverMutationType at all, so, as I remarked, the code you introduced would never actually be used when called by any routine introduced by this patch), but I think it's a confusing way to code it.

If you want, you could define some private method which both ClusterQuiver and QuiverMutationType would call. Or, and maybe this makes even more sense, QuiverMutationType could call ClusterQuiver to build the quiver, but ClusterQuiver would know how to build other quivers that aren't the default quivers associated to any QuiverMutationType.

In either of these cases, I would leave things as they are on this ticket, and make the change of #10538.

comment:120 follow-up: Changed 5 years ago by gmoose05

I have now rolled back my recent changes. I believe there are some advantages to making further changes to the quiver_mutation_type.py file, but these advantages only occur in conjunction with the quiver.py file. Thus, we still should discuss my ideas to see if there's possible bugs that I'm not foreseeing, but these no longer affect #10527, only #10538. I have moved those changes (except for the bugs you found) out of my recent patch.

The general point is that I don't want to add more quiver mutation types, I only want to allow more flexibility in the user building a quiver. Since quiver_mutation_type.py is where we define all the digraphs anyway, my default plan was to eventually define more digraphs in quiver_mutation_type.py through a private method that a user won't directly access. In fact, we already have this programmed, !QuiverMutationType_Irreducible, which is not imported into the global name space. In #10538, I was planning to have a patch that will allow the ClusterQuiver class to call this command for certain user inputs. This is somewhat like the suggestion you just made, if I understood correctly.

Short summary: my patch now only takes care of the issues you recently raised.

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

Replying to gmoose05:

Short summary: my patch now only takes care of the issues you recently raised.

I applied them again, and it worked well -- I agree that it is better to leave the changes on constructing other quivers to the other patch, so we have some time for discussions on design issues...

Replying to robertwb:

Lazily importing * is preferred, as it avoids stat'ing a new directory and loading two new files (init.py and all.py). All this filesystem activity seems to be one of the primary reasons for slow Sage startup time (perhaps even more so than the execution of the .py files themselves) and if we're going to try to fix that it's important to be careful about what continues to be added. That being said, I have know idea what's up with volker-desktop.stp.dias.ie . It's as if the file existed but was incomplete/empty. Let's see if that continues to be a problem.

Thanks for your comments again. Are you suggesting to lazily importing * ? We first did that but the patchbot didn't like it (i.e. some tests failed, see Comments 95 and 97 above), that's why we then did it another way.

comment:122 in reply to: ↑ 121 Changed 5 years ago by robertwb

Replying to robertwb:

Lazily importing * is preferred, as it avoids stat'ing a new directory and loading two new files (init.py and all.py). All this filesystem activity seems to be one of the primary reasons for slow Sage startup time (perhaps even more so than the execution of the .py files themselves) and if we're going to try to fix that it's important to be careful about what continues to be added. That being said, I have know idea what's up with volker-desktop.stp.dias.ie . It's as if the file existed but was incomplete/empty. Let's see if that continues to be a problem.

Thanks for your comments again. Are you suggesting to lazily importing * ? We first did that but the patchbot didn't like it (i.e. some tests failed, see Comments 95 and 97 above), that's why we then did it another way.

Yeah, I looked at those patchbot errors and was wondering if they were something environmental, 'cause I don't see how they could come up. I tried to apply these patches myself to try it out, but I apparently didn't have a new enough pre-release. I was thinking of adding a patch to use import * again and seeing if the patchbots (after checking my own local copy) were still unhappy (and may get around to doing this once I build myself a 5.4 alpha, as 5.3rc0 is apparently too old).

comment:123 Changed 4 years ago by hthomas

Hi Gregg--

Thanks again for revising your patch. The current version looks fine. In the absence of the patchbot, I should make sure the patch applies properly and tests okay on the current development release, not that this is likely to be a problem. My laptop is building 5.4.beta1 now.

cheers,

Hugh

comment:124 follow-up: Changed 4 years ago by hthomas

  • Status changed from needs_review to positive_review

comment:125 in reply to: ↑ 124 Changed 4 years ago by stumpc5

Replying to hthomas:

Thank god (and also Gregg and Hugh)!

comment:126 Changed 4 years ago by jdemeyer

  • Milestone changed from sage-5.4 to sage-5.5

Please clarify which patches have to be applied.

comment:127 Changed 4 years ago by stumpc5

  • Description modified (diff)

Apply only

  • trac_10527-quiver_mutation_type.patch

comment:128 Changed 4 years ago by hthomas

Perhaps this will unconfuse the patchbot.

Apply trac_10527-quiver_mutation_type.patch

comment:129 follow-up: Changed 4 years ago by hthomas

Volker's patchbot's complaint was actually just that a test timed out -- so not something we need to worry about.

Christian, could you please remove the sixth line of the patch, and change the commit message to something more appropriate for the patch as a whole?

cheers,

Hugh

comment:130 in reply to: ↑ 129 Changed 4 years ago by stumpc5

Replying to hthomas:

Christian, could you please remove the sixth line of the patch, and change the commit message to something more appropriate for the patch as a whole?

done.

comment:131 Changed 4 years ago by hthomas

Thanks, Christian. Sorry to be tedious about this -- I think it could have held the process up later, so I figured, better to ask you to fix it now.

Changed 4 years ago by jdemeyer

comment:132 Changed 4 years ago by jdemeyer

Patch rebased to sage-5.4.rc1.

comment:133 Changed 4 years ago by jdemeyer

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