Opened 7 years ago

Closed 6 years ago

#14102 closed enhancement (fixed)

Nonsymmetric Macdonald Polynomials for all affine types

Reported by: bump Owned by: sage-combinat
Priority: major Milestone: sage-6.3
Component: combinatorics Keywords: Nonsymmetric Macdonald polynomials, days40, days45, days49, days54, ICERM2013
Cc: sage-combinat Merged in:
Authors: Nicolas M. Thiéry, Anne Schilling Reviewers: Anne Schilling, Nicolas M. Thiéry, Mark Shimozono, Bogdan Ion
Report Upstream: N/A Work issues: doc-pdf
Branch: a0ceb91 (Commits) Commit: a0ceb91854a6899d33001c984f709ad5758e8317
Dependencies: #4327, #14143, #13589, #10963, #14673, #14610, #14775, #15931 Stopgaps:

Description (last modified by nthiery)

This ticket implements nonsymmetric Macdonald polynomials for arbitrary affine Cartan type (including twisted and BC, but not Koornwinder) using the recursion formula in terms of Demazure-Lusztig and Cherednik operators. It complements the type-A implementation based on the HHL combinatorial formula of #2708.

This patch was written by Anne Schilling and Nicolas M. Thiéry during the ICERM Semester Program on "Automorphic Forms, Combinatorial Representation Theory and Multiple Dirichlet Series" (January 28, 2013 - May 3, 2013) with the help of Dan Bump, Ben Brubaker, Bogdan Ion, Dan Orr, Arun Ram, Siddhartha Sahi, and Mark Shimozono. Special thanks go to Bogdan Ion and Mark Shimozono for their patient explanations and hand computations to check the code.

In a follow-up ticket #14847 Whittaker functions and other features will become available.

Attachments (2)

trac_14102-nonsymmetric-macdonald.patch (228.4 KB) - added by nthiery 7 years ago.
trac_14102-nonsymmetric-macdonald.2.patch (229.9 KB) - added by aschilling 7 years ago.

Download all attachments as: .zip

Change History (96)

comment:1 Changed 7 years ago by bump

  • Description modified (diff)

comment:2 Changed 7 years ago by bump

  • Description modified (diff)

comment:3 Changed 7 years ago by aschilling

  • Keywords polynomials days40 days45 added

comment:4 Changed 7 years ago by aschilling

  • Authors set to Nicolas M. Thiery, Anne Schilling
  • Dependencies set to #4327, #14143, #10963, #14673, #14610
  • Description modified (diff)
  • Keywords days49 added

comment:5 Changed 7 years ago by aschilling

  • Description modified (diff)
  • Reviewers set to Nicolas M. Thiery, Anne Schilling, Mark Shimozono, Bogdan Ion

comment:6 Changed 7 years ago by nthiery

  • Authors changed from Nicolas M. Thiery, Anne Schilling to Nicolas M. Thiéry, Anne Schilling
  • Description modified (diff)
  • Reviewers changed from Nicolas M. Thiery, Anne Schilling, Mark Shimozono, Bogdan Ion to Anne Schilling, Nicolas M. Thiéry, Mark Shimozono, Bogdan Ion
  • Status changed from new to needs_review
  • Summary changed from Nonsymmetric Macdonald Polynomials to Nonsymmetric Macdonald Polynomials for all affine types

comment:7 follow-ups: Changed 7 years ago by mshimo

Anne and Nicolas,

The family returned by NonSymmetricMacdonaldPolynomials? should possess a method that gives direct access to the affine Hecke representation of which it is a basis.

I would like to add symmetric Macdonald polynomials to this patch. It should be straightforward, especially if the AHA representation is at hand.

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

Hi Mark,

The family returned by NonSymmetricMacdonaldPolynomials? should possess a method that gives direct access to the affine Hecke representation of which it is a basis.

Do you mean that you want the operators

        T   = KL.twisted_demazure_lusztig_operators     (   q1, q2, convention="dominant")
        T_Y = KL.demazure_lusztig_operators_on_classical(q, q1, q2, convention="dominant")

directly available? Or in which format do you want the Hecke algebra representation? You can get

sage: E = NonSymmetricMacdonaldPolynomials(["A",2,1])
sage: E._KL
Group algebra of the Ambient space of the Root system of type ['A', 2, 1] over Fraction Field of Multivariate Polynomial Ring in q, q1, q2 over Rational Field

Best,

Anne

comment:9 follow-up: Changed 7 years ago by mshimo

Anne,

The methods I wanted were hidden in the _T and _T_Y attributes.

Why does twisted_demazure_lusztig_operators use classical().dual().affine().dual()? I believe this is correct for untwisted root systems but why do we know this is correct for twisted root systems? If this is a bug it only affects the zero-th Hecke operator.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by nthiery

Replying to mshimo:

The methods I wanted were hidden in the _T and _T_Y attributes.

Ok. Feel free to add whichever accessors you might think relevant.

Why does twisted_demazure_lusztig_operators use classical().dual().affine().dual()? I believe this is correct for untwisted root systems but why do we know this is correct for twisted root systems? If this is a bug it only affects the zero-th Hecke operator.

Well, it's not a bug, since we get the correct results :-)

More seriously: The cartan type as specified here is actually not really used, except for testing for the relations. It indeed looks dubious and should at some point be rewritten using the "other_affinization" method and the like. We must have been lucky to specify a type that gives the same Coxeter group.

So, if you have a short way to specify the cartan type which you believe is more correct, please go ahead. Otherwise I think this can wait until someone reimplements T0_check in a less ad-hock way.

Cheers,

Nicolas

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

Replying to mshimo:

I would like to add symmetric Macdonald polynomials to this patch. It should be straightforward, especially if the AHA representation is at hand.

Cool. Since this patch is quite big already, I guess that could go in a followup ticket like the whittaker function one.

comment:12 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by mshimo

Nicolas,

Well, it's not a bug, since we get the correct results :-)

Do you know of an independent way to compute the nonsymmetric Macdonald polynomial for twisted root systems, not just the t=0 specialization? How was this verified?

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

Hi Mark,

Do you know of an independent way to compute the nonsymmetric Macdonald polynomial for twisted root systems, not just the t=0 specialization? How was this verified?

The code currently checks that the results (i.e. the nonsymmetric Macdonald polynomials) are indeed eigenvectors of the Y operators. These conditions are not easy to satisfy. For types A_n^{(1)}, B_n^{(1)}, BC_n^{(2)} and BC_n^{(2)}-dual the NSMs were carefully compared against Bogdan Ion's hand and maple computations. If you are worried, I can ask him to confirm type A_{2n-1}^{(2)} as well.

Best,

Anne

comment:14 Changed 7 years ago by aschilling

  • Description modified (diff)

comment:15 follow-up: Changed 7 years ago by mshimo

Nicolas,

I added symmetric macdonald polynomials, fixed the parabolic root system stuff, and edited the docs for nonsymmetric macdonalds rather extensively. I already folded my review patch onto the given patch. Please have a look and if it is ok, replace the above exported patch (I don't have sufficient permissions to do so).

--Mark

comment:16 in reply to: ↑ 15 Changed 7 years ago by aschilling

Hi Mark,

Thank you for adding the parabolic root stuff and the symmetric Macdonald polynomials. Usually it is much better not to fold your patch straight in, so that the other people can review your changes first.

Best,

Anne

comment:17 Changed 7 years ago by nthiery

Mark's review patch can be retrieved from the queue history: http://combinat.sagemath.org/patches/rev/f9a14d979bb9

I am reviewing it now.

comment:18 follow-ups: Changed 7 years ago by nthiery

Hi Mark!

Thanks a lot for yor work. The documentation reads much better now. Here are a couple questions about your changes:

 - sage: w = W.from_reduced_word([2,3])
 - sage: w.quantum_bruhat_successors([1,3])
 - Traceback (most recent call last):
 - ...
 - ValueError: s2*s3 is not of minimum length in its coset of the parabolic subgroup
 - generated by the reflections [1, 3] 

Was there a reason for removing this test?

- This implementation, based on the general recursion formula,
- covers all affine types including untwisted, twisted, `BC` and its
- dual.
+ This implementation covers all reduced affine root systems.

The phrasing certainly can be improved, but I think it is useful to briefly state the method used.

The extended affine Hecke algebra contains the Y's

Do we really need to mention the extended affine Hecke algebra? The Y's that are implemented are obtained by compositions of the T's and indexed by (co)roots.

629 + if len(index_set) == 0:
630 + return [] 

Was there a reason for this test in positive_roots? For now I removed it.

I removed the method for sums of parabolic positive roots. Do we still really need "is_parabolic_root"?

I did a couple changes in a review patch which I am about to push.

One remaining thing: I noticed that you changed "non-symmetric" to "nonsymmetric". If this is really what we want, then for consistency we should rename the class to NonsymmetricMacdonaldPolynomials? (and its file accordingly). But that would not be consistent with the e.g. NonCommutativeSymmetricFunctions?.

Cheers,

Nicolas

comment:19 Changed 7 years ago by nthiery

A couple further comments:

About symmetric macdo:

  • Could you add a couple tests comparing them to the other implementation in type A?
  • Do we have other potential consistency checks? Maybe test that they are indeed symmetric?
  • We should make sure that, in the current way they are calculated, the caching happens properly. I.e. when we sum over the orbit, we want that e.g. T_{1,2}(f) reuses T_{1}(f).

Other than that, I would be in favor of extracting the changes about "positive roots" to a separate ticket, since they are mostly unrelated to macdonald polynomials (or are they not?).

comment:20 in reply to: ↑ 18 ; follow-up: Changed 7 years ago by nthiery

Replying to nthiery:

I did a couple changes in a review patch which I am about to push.

Pushed on the queue! Please review!

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

Replying to nthiery:

Replying to nthiery:

I did a couple changes in a review patch which I am about to push.

Pushed on the queue! Please review!

I cannot see that you recently pushed.

Anne

comment:22 in reply to: ↑ 18 ; follow-up: Changed 7 years ago by mshimo

Replying to nthiery:

 - sage: w = W.from_reduced_word([2,3])
 - sage: w.quantum_bruhat_successors([1,3])
 - Traceback (most recent call last):
 - ...
 - ValueError: s2*s3 is not of minimum length in its coset of the parabolic subgroup
 - generated by the reflections [1, 3] 

Was there a reason for removing this test?

I got a doctest error I didn't understand (stupid reason): the tester was producing a blank line that wasn't matching the doctest string.

- This implementation, based on the general recursion formula,
- covers all affine types including untwisted, twisted, `BC` and its
- dual.
+ This implementation covers all reduced affine root systems.

The phrasing certainly can be improved, but I think it is useful to briefly state the method used.

I can add that (or rather put it back in).

The extended affine Hecke algebra contains the Y's

Do we really need to mention the extended affine Hecke algebra? The Y's that are implemented are obtained by compositions of the T's and indexed by (co)roots.

If only coroots are involved then no. I can take that out.

629 + if len(index_set) == 0:
630 + return [] 

Was there a reason for this test in positive_roots? For now I removed it.

It is not needed (I added it during debugging before finding that the initial set of simple roots needed to be changed before the search).

I removed the method for sums of parabolic positive roots. Do we still really need "is_parabolic_root"?

That sum method should be thought of as analogous to the computation of rho. This fixed quantity (depends only on the root system and subset of index set) gets used many times over in the computation of the parabolic quantum Bruhat graph (which we then use to make KR crystals). That is why I decided not to delete it. There is a method in root_systems/root_space.py called quantum_root that requires this sum as well.

is_parabolic_root is probably not really needed.

I did a couple changes in a review patch which I am about to push.

One remaining thing: I noticed that you changed "non-symmetric" to "nonsymmetric". If this is really what we want, then for consistency we should rename the class to NonsymmetricMacdonaldPolynomials? (and its file accordingly). But that would not be consistent with the e.g. NonCommutativeSymmetricFunctions?.

Not really. I'm generally against unnecessary hyphenation. (By the way I like NoncommutativeSymmetricFunctions? better than the alternative.)

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

I cannot see that you recently pushed.

Argl, sorry. I had cloned from a local repository (to avoid messing with my local C3 changes), and did not notice I was thus pushing back there ...

Done!

Cheers,

Nicolas

comment:24 in reply to: ↑ 22 ; follow-up: Changed 7 years ago by nthiery

Replying to mshimo:

I removed the method for sums of parabolic positive roots. Do we still really need "is_parabolic_root"?

That sum method should be thought of as analogous to the computation of rho. This fixed quantity (depends only on the root system and subset of index set) gets used many times over in the computation of the parabolic quantum Bruhat graph (which we then use to make KR crystals). That is why I decided not to delete it.

It's used many times there indeed, but if I remember well calculated only once at the beginning of the method.

Ok. If I recall correctly my review patch has taken it out.

May I let you implement the little things on which we agreed, and possibly reinstate sums_of_parabolic_positive_roots, with a cache and documentation, if you think it's really worthwhile?

I'm generally against unnecessary hyphenation.

I see your point.

Not totally consistent.

(By the way I like NoncommutativeSymmetricFunctions? better than the alternative.)

On the other hand NCSF has been the traditional shorthand for a while ...

Hmm. Here is what we have in Sage currently:

NonCommutativeSymmetricFunctions? NonDecreasingParkingFunction? NonDecreasingParkingFunctions? NonNegativeIntegerSemiring? NonNegativeIntegers? NonSymmetricMacdonaldPolynomials? NonattackingFillings?

There is a bias, but it's not perfectly consistent.

Do we need to make a poll on sage-devel?

Cheers,

Nicolas

comment:25 in reply to: ↑ 24 Changed 7 years ago by mshimo

Nicolas,

Right now the quantum bruhat graph is twice broken; I will repair it.

The quantum_bruhat_successors method in categories/weyl_groups (element method) gets called repeatedly (once for each element of the parabolic quotient) to generate the quantum Bruhat graph. Each time this function is called, it needs to loop through the set of positive roots that are NOT in the parabolic subsystem; each such root could lead to a quantum arrow. For this reason I'm going to reinstate the function that constructs the non-parabolic positive roots. At this point I don't see a reason why one should also not remember the sum of these roots (rather than recompute it repeatedly).

Also the quantum_root method (in root_systems/root_space.py), which is called by quantum_bruhat_successors, requires the same sum.

I'll put the failing doctest back in. I couldn't match the exception message, which started with a <BLANKLINE> rather than Traceback. Maybe you can match it.

--Mark

comment:26 follow-up: Changed 7 years ago by mshimo

I pushed another review patch after Nicolas' review patch.

I figured out the doctest issue.

There is a direct formula (see Cherednik's 2009 book, formula (3.3.15)) for the symmetric macdonald as a linear combination of nonsymmetric Macdonalds (with complicated rational function coefficients), which someone else can program when they feel inspired. This is the same kind of chore as writing a function for the precise eigenvalues in the nonsymmetric Macdonald recurrence.

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

Hi Mark and Nicolas,

Thank you both for your review patches. I looked them both over and in principle it is fine with me to fold them in (I made some minor fixes to Mark's review patch).

Regarding the naming, I agree with Mark that it is nicer to remove the hyphens. However, can we nonetheless leave the name of the class as NonSymmetricMacdonaldPolynomials? in accordance with NCSF and most other classes? I do not mind this convention in sage since the real meaning is in (non)symmetric and not the non. But if you really want to change it, that is fine with me as well.

Best,

Anne

comment:28 in reply to: ↑ 27 Changed 7 years ago by aschilling

Hi Mark and Nicolas,

Here is an update on the twisted types. I asked Bogdan Ion for data for type A_5^{(2)}. Everything seems to match:

sage: K = QQ['q,t'].fraction_field()
sage: q,t = K.gens()
sage: E = NonSymmetricMacdonaldPolynomials(["A",5,2], q, t**2,-1)
sage: omega = E.keys()
sage: vars = K['x1,x2,x3'].gens()
sage: x1,x2,x3 = K['x1,x2,x3'].gens()

sage: E[omega[1]].expand(vars) == x1
True
sage: E[-omega[1]].expand(vars) == (t-1)*(t+1)*(q*t^10-1)*x1/((t^2*q-1)*(q*t^5-1)*(q*t^5+1))+(t-1)*(t+1)*x2/(t^2*q-1)+(t-1)*(t+1)*x3/(t^2*q-1)+(t-1)*(t+1)/((t^2*q-1)*x2)+1/x1+(t-1)*(t+1)/((t^2*q-1)*x3)
True
sage: E[omega[2]].expand(vars) == (t-1)*(t+1)*x1/(q*t^8-1)+x2
True
sage: E[-omega[2]].expand(vars) == (q*t^6+1)*(t-1)*(t+1)*(q*t^6-1)*x1/((q*t^5-1)*(q*t^5+1)*(q*t^4-1))+(t+1)*(t-1)*(q*t^8-1)*x2/((q*t^5-1)*(q*t^5+1)*(q*t^4-1))+(t-1)*(t+1)*x3/(q*t^4-1)+1/x2+(t-1)*(t+1)/((q*t^4-1)*x3)
True
sage: E[-omega[1]-omega[2]].expand(vars) == (t-1)*(t+1)*(q^3*t^10+2*t^10*q^2-q^2*t^8-2*t^6*q^2+q*t^8-q*t^6-2*q*t^4+q+2-t^4)/((t^2*q-1)*(q*t^4-1)*(q*t^3-1)*(q*t^3+1))+(t+1)*(t-1)/((t^2*q-1)*x3*x2)+(t+1)*(t-1)*x3/((t^2*q-1)*x2)+(t+1)^2*(t-1)^2*(q*t^4+t^2+1)*x1*x3/((q*t^3-1)*(q*t^3+1)*(t^2*q-1))+(t+1)*(t-1)*(q*t^6-1)*x1/((q*t^3-1)*(q*t^3+1)*(t^2*q-1)*x2)+(t+1)^2*(t-1)^2*(q*t^4+t^2+1)*x1/((q*t^3-1)*(q*t^3+1)*(t^2*q-1)*x3)+(t+1)*(t-1)*(t^10*q^2-q*t^8+t^8-q*t^6-t^4+1)*x1*x2/((t^2*q-1)*(q*t^4-1)*(q*t^3-1)*(q*t^3+1))+(t+1)*(t-1)*(q*t^6-1)*x2/((q*t^3-1)*(q*t^3+1)*(t^2*q-1)*x1)+(t+1)^2*(t-1)^2*(q*t^4+t^2+1)*x3*x2/((q*t^3-1)*(q*t^3+1)*(t^2*q-1))+1/(x1*x2)+(t+1)*(t-1)*x3/((t^2*q-1)*x1)+(t+1)*(t-1)/((t^2*q-1)*x1*x3)+(t+1)^2*(t-1)^2*(q*t^4+t^2+1)*x2/((q*t^3-1)*(q*t^3+1)*(t^2*q-1)*x3)
True
sage: E[omega[1]-omega[2]].expand(vars) == (q*t^6+1)*(t-1)*(t+1)*(q*t^6-1)*q/((q*t^5-1)*(q*t^5+1)*(q*t^4-1))+(t-1)*(t+1)*x1*x3/(q*t^4-1)+x1/x2+(t-1)*(t+1)*x1/((q*t^4-1)*x3)+(t+1)*(t-1)*(q*t^8-1)*x1*x2/((q*t^5-1)*(q*t^5+1)*(q*t^4-1))Truesage: E[omega[3]].expand(vars) == (t-1)*(t+1)*x1/(q*t^6-1)+(t-1)*(t+1)*x2/(q*t^6-1)+x3True
sage: E[-omega[3]].expand(vars) == (q*t^6+1)*(t-1)*(t+1)*x1/((q*t^5-1)*(q*t^5+1))+(q*t^6+1)*(t-1)*(t+1)*x2/((q*t^5-1)*(q*t^5+1))+(t-1)*(t+1)*x3/((q*t^5-1)*(q*t^5+1))+1/x3
True

I put a review patch putting (similar) tests for twisted types as a review patch.

Nicolas, please review everything, fold and upload the updated patch!

Thanks,

Anne

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

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

comment:30 in reply to: ↑ 29 Changed 7 years ago by aschilling

Hi Nicolas and Mark,

I made some mini changes, but other than that everything looks good now!

Anne

comment:31 follow-up: Changed 7 years ago by mshimo

Why does the patchbot see red? Does it mean the patches don't apply?

comment:32 in reply to: ↑ 31 Changed 7 years ago by aschilling

  • Dependencies changed from #4327, #14143, #10963, #14673, #14610 to #4327, #14143, #13589, #10963, #14673, #14610

Replying to mshimo:

Why does the patchbot see red? Does it mean the patches don't apply?

The patch depends on #10963 and Nicolas probably has not yet uploaded that patch yet on trac. Oops and #13589 is also a prerequiste. So I put this into the dependencies.

Last edited 7 years ago by aschilling (previous) (diff)

comment:33 Changed 7 years ago by nthiery

  • Description modified (diff)

comment:34 Changed 7 years ago by nthiery

Hi!

I checked Anne's little review patch.

I replaced here my "old" patch by yours.

I just posted on trac the latest #10963 patch (it indeed needed some rebasing on top of the reviewer's patches for #13589)

Cheers,

Nicolas

comment:35 Changed 7 years ago by mshimo

Nicolas, I updated the patch on the combinat queue since there was a subtle bug in the symmetric macdonald polynomials having to do with caching and the order of searching a Weyl orbit. Didn't export it to trac.

Again I forgot to make a separate review patch. There is a doctest change in symmetric_macdonald polynomials and a change in the orbit method of root_lattice_realizations, to calling TransitiveIdealGraded? instead of TransitiveIdeal?.

--Mark

comment:36 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-pending

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

  • Description modified (diff)

comment:38 in reply to: ↑ 37 Changed 7 years ago by aschilling

Ok, Mark, I updated the patch on the trac server!

Anne

Changed 7 years ago by nthiery

comment:39 Changed 7 years ago by nthiery

  • Dependencies changed from #4327, #14143, #13589, #10963, #14673, #14610 to #4327, #14143, #13589, #10963, #14673, #14610, #14755
  • Description modified (diff)

The updated patch has been trivially rebased on top of 14755 which is about to get merged.

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

  • Dependencies changed from #4327, #14143, #13589, #10963, #14673, #14610, #14755 to #4327, #14143, #13589, #10963, #14673, #14610, #14775

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

I think you meant #14775 instead of #14755, so I changed that!

Changed 7 years ago by aschilling

comment:42 in reply to: ↑ 41 Changed 7 years ago by aschilling

Apply trac_14102-nonsymmetric-macdonald.2.patch

Fixed broken doctests in pieri_factors.py that must have sneaked in during the last 3 months.

comment:43 Changed 7 years ago by aschilling

  • Description modified (diff)

comment:44 Changed 7 years ago by aschilling

  • Branch set to public/combinat/nonsymmetric_macdonald-14102
  • Commit set to dfc1fe8876f0eca75aaad312fb197edf823fde20

New commits:

dfc1fe8#14102: Nonsymmetric Macdonald polynomials
362fd5e# Tue Oct 29 20:14:19 2013 +0100
b2914f3# Sun Oct 27 13:58:49 2013 +0100
9d9cae3# Sat Oct 19 11:50:04 2013 +0200
0251a33Trac #13394: Implement faster and safer WeakValueDictionary?

comment:45 Changed 7 years ago by aschilling

  • Keywords days54 added

comment:46 Changed 6 years ago by vbraun

I'm getting occasional random failures

sage -t --long src/sage/combinat/root_system/non_symmetric_macdonald_polynomials.py
**********************************************************************
File "src/sage/combinat/root_system/non_symmetric_macdonald_polynomials.py", line 200, in sage.combinat.root_system.non_symmetric_macdonald_polynomials.NonSymmetricMacdonaldPolynomials
Failed example:
    T._test_relations()                 # long time (1.3s)
Exception raised:
    Traceback (most recent call last):
      File "/Users/buildbot/build/sage/bsd-1/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 480, in _run
        self.execute(example, compiled, test.globs)
      File "/Users/buildbot/build/sage/bsd-1/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 839, in execute
        exec compiled in globs
      File "<doctest sage.combinat.root_system.non_symmetric_macdonald_polynomials.NonSymmetricMacdonaldPolynomials[30]>", line 1, in <module>
        T._test_relations()                 # long time (1.3s)
      File "/Users/buildbot/build/sage/bsd-1/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/root_system/hecke_algebra_representation.py", line 471, in _test_relations
        tester.assertEqual(x, y)
      File "/Users/buildbot/build/sage/bsd-1/sage_git/build/local/lib/python/unittest/case.py", line 515, in assertEqual
        assertion_func(first, second, msg=msg)
      File "/Users/buildbot/build/sage/bsd-1/sage_git/build/local/lib/python/unittest/case.py", line 508, in _baseAssertEqual
        raise self.failureException(msg)
    AssertionError: (-q1^2*q2^2-q1*q2^3)*B[-e[0] - e[1] + 2*e['delta']] + (-q1^2*q2^2-q1*q2^3)*B[e[0] + e['delta'] - e[1]] + (-q1*q2^3)*B[-2*e[1] + 2*e['delta']] + (-q1^2*q2^2-q1*q2^3)*B[e['delta']] != (-q1^2*q2^2-2*q1*q2^3-q2^4)*B[-e[0] - e[1] + 2*e['delta']] + (q1*q2^3+q2^4)*B[-e[0] + 2*e['delta'] - e[1]] + (-q1^2*q2^2-q1*q2^3)*B[e[0] - e[1] + e['delta']] + (-q1*q2^3)*B[-2*e[1] + 2*e['delta']] + (-q1^2*q2^2-q1*q2^3)*B[e['delta']]

comment:47 Changed 6 years ago by nthiery

Yikes! Thanks Volker for the report.

On the right hand side, the index -e[0] - e[1] + 2*e['delta'] appears twice; collected together one would get the same value as on the left hand side. So now we need to figure out why the two occurences of -e[0] - e[1] + 2*e['delta'] are not detected as equal. For this, I would need to reproduce the error in order to introspect the values in a debugger.

Volker: did you notice any pattern for when the error occurs (platform, frequency)? Or should I just launch the test enough times on any platform and it should occur at some point?

Cheers,

comment:48 follow-up: Changed 6 years ago by vbraun

This was on the snapperkob (Ubuntu x86_64) and bsd (OSX 10.6) buildbot, so shouldn't be a problem to reproduce if you run it in a loop...

comment:49 Changed 6 years ago by git

  • Commit changed from dfc1fe8876f0eca75aaad312fb197edf823fde20 to 9d365416bb2bceae585b0c456d2ea2abc758f94f
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. Last 10 new commits:

98a4407rename summand_* methods of Cartesian products
3e2003dReviewed Peter's renaming of summand_* methods of Cartesian products, using deprecated_function_alias
c718f21#10963: typo fix 12963 -> 10963
56c8eaaMerge branch 'develop' into ticket/10963
7ab3103Fixing some typos in the category primer and category_with_axioms
f1b6804Merge branch 'public/ticket/10963-doc-distributive' of trac.sagemath.org:sage into public/ticket/10963-doc-distributive
977a940Axioms: minor improvements here and there, and reworked explanations about the binding behavior of categories with axioms
acd58b3Fix failing tests due to little conflict when merging with #12141 (finite dimensional algebra)
ece5c97Axioms: fixed merge with #15873
9d36541Merge branch 'public/ticket/10963-doc-distributive' into u/aschilling/combinat/nonsymmetric_macdonald-14102

comment:50 in reply to: ↑ 48 ; follow-up: Changed 6 years ago by aschilling

I rebased on the latest version of 10963 and now get the following doctest failures:

sage -t coxeter_group.py
**********************************************************************
File "coxeter_group.py", line 69, in sage.combinat.root_system.coxeter_group.CoxeterGroup
Failed example:
    W
Expected:
    Coxeter group over Universal Cyclotomic Field with Coxeter matrix:
    [1 3 2]
    [3 1 5]
    [2 5 1]
Got:
    Permutation Group with generators [(2,6)(3,18)(4,8)(5,9)(7,10)(11,12)(13,14)(17,21)(19,23)(20,24)(22,25)(26,27)(28,29), (1,4)(2,17)(3,6)(5,7)(9,11)(10,12)(14,15)(16,19)(18,21)(20,22)(24,26)(25,27)(29,30), (1,16)(2,5)(4,7)(6,9)(8,10)(11,13)(12,14)(17,20)(19,22)(21,24)(23,25)(26,28)(27,29)]
**********************************************************************
1 item had failures:
   1 of  18 in sage.combinat.root_system.coxeter_group.CoxeterGroup
    [29 tests, 1 failure, 15.61 s]
sage -t coxeter_matrix.py
    [10 tests, 0.42 s]
sage -t dynkin_diagram.py
    [104 tests, 0.91 s]
sage -t hecke_algebra_representation.py
**********************************************************************
File "hecke_algebra_representation.py", line 344, in sage.combinat.root_system.hecke_algebra_representation.HeckeAlgebraRepresentation.Tw
Failed example:
    x = KW.an_element(); x
Expected:
    B[123121] + 2*B[12312] + 3*B[12321] + B[123]
Got:
    2*B[12321] + 3*B[1231] + B[123] + B[]
**********************************************************************
File "hecke_algebra_representation.py", line 726, in sage.combinat.root_system.hecke_algebra_representation.HeckeAlgebraRepresentation.Y_eigenvectors
Failed example:
    [E[w] for w in W]
Expected:
    [B[2121],
    -B[2121] + B[121],
    -B[2121] + B[212],
    (q2/(-q1+q2))*B[2121] + ((-q2)/(-q1+q2))*B[121] - B[212] + B[12],
    ((-q2^2)/(-q1^2+q1*q2-q2^2))*B[2121] - B[121] + (q2^2/(-q1^2+q1*q2-q2^2))*B[212] + B[21],
    ((q1^2+q2^2)/(-q1^2+q1*q2-q2^2))*B[2121] + ((-q1^2-q2^2)/(-q1^2+q1*q2-q2^2))*B[121] + ((-q2^2)/(-q1^2+q1*q2-q2^2))*B[212] + (q2^2/(-q1^2+q1*q2-q2^2))*B[12] - B[21] + B[1],
    (q2/(q1-q2))*B[2121] + (q2/(-q1+q2))*B[121] + (q2/(-q1+q2))*B[212] - B[12] + ((-q2)/(-q1+q2))*B[21] + B[2],
    B[2121] - B[121] - B[212] + B[12] + B[21] - B[1] - B[2] + B[]]
Got:
    [B[2121] - B[121] - B[212] + B[12] + B[21] - B[1] - B[2] + B[], -B[2121] + B[212], (q2/(q1-q2))*B[2121] + (q2/(-q1+q2))*B[121] + (q2/(-q1+q2))*B[212] - B[12] + ((-q2)/(-q1+q2))*B[21] + B[2], ((-q2^2)/(-q1^2+q1*q2-q2^2))*B[2121] - B[121] + (q2^2/(-q1^2+q1*q2-q2^2))*B[212] + B[21], ((q1^2+q2^2)/(-q1^2+q1*q2-q2^2))*B[2121] + ((-q1^2-q2^2)/(-q1^2+q1*q2-q2^2))*B[121] + ((-q2^2)/(-q1^2+q1*q2-q2^2))*B[212] + (q2^2/(-q1^2+q1*q2-q2^2))*B[12] - B[21] + B[1], B[2121], (q2/(-q1+q2))*B[2121] + ((-q2)/(-q1+q2))*B[121] - B[212] + B[12], -B[2121] + B[121]]
**********************************************************************
2 items had failures:
   1 of  19 in sage.combinat.root_system.hecke_algebra_representation.HeckeAlgebraRepresentation.Tw
   1 of  15 in sage.combinat.root_system.hecke_algebra_representation.HeckeAlgebraRepresentation.Y_eigenvectors
    [286 tests, 2 failures, 7.72 s]

comment:51 follow-up: Changed 6 years ago by tscrim

Do you have Coxeter3 installed? This might account for the first error (in which case, it is an unrelated error I need to fix). The latter two errors seem to be reordering, and if so, likely comes from the version bump of GAP.

Last edited 6 years ago by tscrim (previous) (diff)

comment:52 in reply to: ↑ 51 Changed 6 years ago by aschilling

Replying to tscrim:

Do you have Coxeter3 installed? This might account for the first error (in which case, it is an unrelated error I need to fix). The latter two errors seem to be reordering, and if so, likely comes from the version bump of GAP.

I do have coxeter3 installed. The second error is not a reordering, but I guess just a different element.

comment:53 Changed 6 years ago by nthiery

Thanks for doing the merge!

I'll work on those failing tests and more importantly on the issue reported by Volker as soon as I can raise the head from #10963.

comment:54 Changed 6 years ago by vbraun

The new GAP does indeed pick different generators for groups. In fact, this happened a while ago and we missed a number of GAP version updates.

comment:55 follow-up: Changed 6 years ago by git

  • Commit changed from 9d365416bb2bceae585b0c456d2ea2abc758f94f to 96ae730a0475ac99cb6c0119d58c9f659e612f1e

Branch pushed to git repo; I updated commit sha1. New commits:

96ae730updated docs in hecke_algebra_representation

comment:56 in reply to: ↑ 55 Changed 6 years ago by aschilling

I get similar errors to what Volker indicated:

sage -t --long non_symmetric_macdonald_polynomials.py
**********************************************************************
File "non_symmetric_macdonald_polynomials.py", line 200, in sage.combinat.root_system.non_symmetric_macdonald_polynomials.NonSymmetricMacdonaldPolynomials
Failed example:
    T._test_relations()                 # long time (1.3s)
Exception raised:
    Traceback (most recent call last):
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 480, in _run
        self.execute(example, compiled, test.globs)
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 839, in execute
        exec compiled in globs
      File "<doctest sage.combinat.root_system.non_symmetric_macdonald_polynomials.NonSymmetricMacdonaldPolynomials[30]>", line 1, in <module>
        T._test_relations()                 # long time (1.3s)
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/combinat/root_system/hecke_algebra_representation.py", line 471, in _test_relations
        tester.assertEqual(x, y)
      File "/Applications/sage/local/lib/python/unittest/case.py", line 515, in assertEqual
        assertion_func(first, second, msg=msg)
      File "/Applications/sage/local/lib/python/unittest/case.py", line 508, in _baseAssertEqual
        raise self.failureException(msg)
    AssertionError: (-q1^2*q2^2-q1*q2^3)*B[-e[0] - e[1] + 2*e['delta']] + (-q1^2*q2^2-q1*q2^3)*B[e[0] + e['delta'] - e[1]] + (-q1*q2^3)*B[-2*e[1] + 2*e['delta']] + (-q1^2*q2^2-q1*q2^3)*B[e['delta']] != (-q1^2*q2^2-2*q1*q2^3-q2^4)*B[-e[0] - e[1] + 2*e['delta']] + (q1*q2^3+q2^4)*B[-e[0] + 2*e['delta'] - e[1]] + (-q1^2*q2^2-q1*q2^3)*B[e[0] - e[1] + e['delta']] + (-q1*q2^3)*B[-2*e[1] + 2*e['delta']] + (-q1^2*q2^2-q1*q2^3)*B[e['delta']]
**********************************************************************
1 item had failures:
   1 of 432 in sage.combinat.root_system.non_symmetric_macdonald_polynomials.NonSymmetricMacdonaldPolynomials
    [562 tests, 1 failure, 453.14 s]
----------------------------------------------------------------------
sage -t --long non_symmetric_macdonald_polynomials.py  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 453.4 seconds
    cpu time: 451.7 seconds
    cumulative wall time: 453.1 seconds

comment:57 in reply to: ↑ 50 Changed 6 years ago by aschilling

Replying to aschilling:

I rebased on the latest version of 10963 and now get the following doctest failures:

sage -t coxeter_group.py
**********************************************************************
File "coxeter_group.py", line 69, in sage.combinat.root_system.coxeter_group.CoxeterGroup
Failed example:
    W
Expected:
    Coxeter group over Universal Cyclotomic Field with Coxeter matrix:
    [1 3 2]
    [3 1 5]
    [2 5 1]
Got:
    Permutation Group with generators [(2,6)(3,18)(4,8)(5,9)(7,10)(11,12)(13,14)(17,21)(19,23)(20,24)(22,25)(26,27)(28,29), (1,4)(2,17)(3,6)(5,7)(9,11)(10,12)(14,15)(16,19)(18,21)(20,22)(24,26)(25,27)(29,30), (1,16)(2,5)(4,7)(6,9)(8,10)(11,13)(12,14)(17,20)(19,22)(21,24)(23,25)(26,28)(27,29)]
**********************************************************************

See #15910 regarding this doctest failure. It has nothing to do with this ticket.

comment:58 follow-up: Changed 6 years ago by nthiery

Hi!

Thanks to Jean-Baptiste's mac, I have been able to reproduce and analyze the bug reported by Volker. At the end of the day, it's a combination of:

  • CombinatorialFreeModule? being somewhat lax in input checking, leading to things like mixes of int's and Integer's in the support of elements.
  • The root system code introducing those int's and Integer's in the first place
  • sort giving different results for mixed lists containing strings and integers, depending on whether some of those integers are int's or Integer's

Thus the odds of things going wrong were low, which is why this remained unnoticed for so long. Why did this only appear on certain platforms? Well, the result of

    sage: sorted([int(0), 'delta', 1])
    [0, 1, 'delta']

is not the same on my Linux and Jean-Baptiste's Mac.

Probably, with some other doctests, the problem would have instead appeared on my machine and not on others.

I assume that (finally!) implementing a proper hash function for CombinatorialFreeModule? elements should do the job. But that will be for another day. Yawn ....

In any cases, no worry to have: as I expected this has nothing to do with #10963.

Cheers,

Nicolas

comment:59 in reply to: ↑ 58 Changed 6 years ago by aschilling

Great that you figured it out!

Anne

comment:60 Changed 6 years ago by git

  • Commit changed from 96ae730a0475ac99cb6c0119d58c9f659e612f1e to ea4af15ac39244296094c00eef12ad0204298f01

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

8e83e42Axioms: primer: specifying the category of a parent and using it to add code + promise thingy
5ab0a43Axioms: improvements to the primer and documentation following Darij's suggestions
d6a0e60Axioms: Fixed some unused imports in categories
914d5f0Axioms: documentation: systematic check about full vs non full subcategories + minor improvements
3e6ced8Merge branch 'categories/axioms-10963' into ticket/14102
1afc63bAxioms: fixed documentation compilation
82702c9Merge branch 'ticket/10963' into ticket/14102
e8fe5ebTrac 15931: implement a proper hash function for (combinatorial) free module elements
e6ef605Trac 15931: add a @cached_method on the new hash function for (combinatorial) free module elements
ea4af15Merge branch 'ticket/15931-hash-free_module' into ticket/14102

comment:61 Changed 6 years ago by nthiery

  • Created changed from 02/12/13 15:58:07 to 02/12/13 15:58:07
  • Modified changed from 03/13/14 16:37:01 to 03/13/14 16:37:01

I merged in the latest version of #10963 (it was a pain to recompile everything everytime I switched between this, develop, or #10963; and

I also merged in #15931, which should fix the bug reported on 46. As a bonus:

sage -t non_symmetric_macdonald_polynomials.py # Before
    [533 tests, 54.28 s]
sage -t non_symmetric_macdonald_polynomials.py # After
    [533 tests, 41.98 s]

Anne: could you test?

comment:62 follow-up: Changed 6 years ago by nthiery

  • Dependencies changed from #4327, #14143, #13589, #10963, #14673, #14610, #14775 to #4327, #14143, #13589, #10963, #14673, #14610, #14775, #15931
  • Description modified (diff)

comment:63 in reply to: ↑ 62 ; follow-up: Changed 6 years ago by aschilling

With the current version, the documentation does not build:

[combinat ] reading sources... [ 99%] tableaux
[combinat ] reading sources... [100%] words
[combinat ] <autodoc>:0: WARNING: Block quote ends without a blank line; unexpected unindent.
[combinat ] <autodoc>:0: WARNING: Block quote ends without a blank line; unexpected unindent.
Traceback (most recent call last):
  File "/Applications/sage/src/doc/common/builder.py", line 83, in f
    execfile(sys.argv[0])
  File "/Applications/sage/src/doc/common/custom-sphinx-build.py", line 185, in <module>
    sphinx.cmdline.main(sys.argv)
  File "/Applications/sage/local/lib/python2.7/site-packages/Sphinx-1.1.2-py2.7.egg/sphinx/cmdline.py", line 206, in main
    print >>error
  File "/Applications/sage/src/doc/common/custom-sphinx-build.py", line 165, in write
    self._write(str)
  File "/Applications/sage/src/doc/common/custom-sphinx-build.py", line 139, in _write
    self._log_line(line)
  File "/Applications/sage/src/doc/common/custom-sphinx-build.py", line 108, in _log_line
    raise OSError(line)
OSError: [combinat ] <autodoc>:0: WARNING: Block quote ends without a blank line; unexpected unindent.


[rings    ] reading sources... [ 92%] sage/rings/quotient_ring_element
Error building the documentation.

Note: incremental documentation builds sometimes cause spurious
error messages. To be certain that these are real errors, run
"make doc-clean" first and try again.
Traceback (most recent call last):
  File "/Applications/sage/src/doc/common/builder.py", line 1474, in <module>
    getattr(get_builder(name), type)()
  File "/Applications/sage/src/doc/common/builder.py", line 269, in _wrapper
    getattr(get_builder(document), 'inventory')(*args, **kwds)
  File "/Applications/sage/src/doc/common/builder.py", line 485, in _wrapper
    x.get(99999)
  File "/Applications/sage/local/lib/python/multiprocessing/pool.py", line 554, in get
    raise self._value
OSError: [combinat ] <autodoc>:0: WARNING: Block quote ends without a blank line; unexpected unindent.

make: *** [doc-html] Error 1

But the long test indeed now passes:

anne$ sage -t --long non_symmetric_macdonald_polynomials.py 
Running doctests with ID 2014-03-13-11-44-46-13a52b2e.
Doctesting 1 file.
sage -t --long non_symmetric_macdonald_polynomials.py
    [562 tests, 273.63 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 273.9 seconds
    cpu time: 269.6 seconds
    cumulative wall time: 273.6 seconds

comment:64 in reply to: ↑ 63 Changed 6 years ago by aschilling

Actually, with the current version of this patch, sage does not even start for me

byte-compiling /Applications/sage/local/lib/python2.7/site-packages/sage/symbolic/expression_conversions.py to expression_conversions.pyc
byte-compiling /Applications/sage/local/lib/python2.7/site-packages/sage/symbolic/function_factory.py to function_factory.pyc
byte-compiling /Applications/sage/local/lib/python2.7/site-packages/sage/symbolic/integration/integral.py to integral.pyc
byte-compiling /Applications/sage/local/lib/python2.7/site-packages/sage/version.py to version.pyc
running install_egg_info
Removing /Applications/sage/local/lib/python2.7/site-packages/sage-6.2.beta4-py2.7.egg-info
Writing /Applications/sage/local/lib/python2.7/site-packages/sage-6.2.beta4-py2.7.egg-info
┌────────────────────────────────────────────────────────────────────┐
│ Sage Version 6.2.beta3, Release Date: 2014-03-03                   │
│ Type "notebook()" for the browser-based notebook interface.        │
│ Type "help()" for help.                                            │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
Traceback (most recent call last):
  File "/Applications/sage/src/bin/sage-ipython", line 6, in <module>
    from sage.misc.interpreter import SageTerminalApp
  File "/Applications/sage/local/lib/python2.7/site-packages/sage/misc/interpreter.py", line 158, in <module>
    from IPython.terminal.interactiveshell import TerminalInteractiveShell
ImportError: No module named terminal.interactiveshell

comment:65 Changed 6 years ago by nthiery

Hmm, Sage starts on my machine. Let's see: does Sage start for you in the branch develop?

I am investigating the doc issue (I in fact get the same problem here).

Last edited 6 years ago by nthiery (previous) (diff)

comment:66 follow-up: Changed 6 years ago by tscrim

Anne, it looks like you have mixed Sage versions. Did you run make build? Judging from the error, you need to rebuild IPython (it was upgraded in 6.2.beta4).

comment:67 in reply to: ↑ 66 Changed 6 years ago by aschilling

Replying to tscrim:

Anne, it looks like you have mixed Sage versions. Did you run make build? Judging from the error, you need to rebuild IPython (it was upgraded in 6.2.beta4).

Yes, I did. Well, something was messed up with my sage. I reinstalled from scratch and ran the tests and now everything works!

comment:68 Changed 6 years ago by aschilling

  • Status changed from needs_review to positive_review

comment:69 Changed 6 years ago by git

  • Commit changed from ea4af15ac39244296094c00eef12ad0204298f01 to 2837531ce92b27ba95fc5e1c96995365c41f8fc1
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

2837531Trac 14102: fixed broken non raw docstring + typo

comment:70 Changed 6 years ago by git

  • Commit changed from 2837531ce92b27ba95fc5e1c96995365c41f8fc1 to 741d5a4fbcae4659425de8948be814377026f376

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

338cc5btrac #16269: Reviewer's remarks
8ac32c2#16280: Fix call for FiniteEnumeratedSet's of plain Python objects
4e27454#16280: Trivial doctest fixes
f67d82etrac #16269: Merged with #16280
0a54b68Trac 16269: little improvements + line folding in the doctest output
b5ad803Trac 16269 (or follow up): optimize CartesianProduct._cartesian_product_of_elements
d2bc8b2Merge cartesian product functionalities from '#16269 and #16288' into categories/axioms-10963
e9c3586Some fixes to the docs and to comments in the code (review patch)
4b21473Merge branch 'public/ticket/10963-doc-distributive' of trac.sagemath.org:sage into public/ticket/10963-doc-distributive
741d5a4Merge with latest #10963.

comment:71 Changed 6 years ago by git

  • Commit changed from 741d5a4fbcae4659425de8948be814377026f376 to 1579b88ae54981d43bb709b370dfed9c9ad16a33

Branch pushed to git repo; I updated commit sha1. New commits:

1579b88Added back in import statement as per comment.

comment:72 Changed 6 years ago by tscrim

I've rebased this off the latest #10963. I've also added back in the import statement in categories/weyl_groups.py in order for the pieri_factors.py tests to pass. I believe this was removed prior to my merge, so I'm asking for someone (probably with coxeter3 installed) to double-check everything works.

comment:73 follow-up: Changed 6 years ago by git

  • Commit changed from 1579b88ae54981d43bb709b370dfed9c9ad16a33 to c6d57ec0a5207d39d08ae01f6b8555623cea9927

Branch pushed to git repo; I updated commit sha1. New commits:

c6d57ecfixed new syntax for crystals in nonsymmetric_macdonald

comment:74 in reply to: ↑ 73 Changed 6 years ago by aschilling

Ok, thanks Travis. There were a couple of failing doctests due to the new crystal syntax. I fixed those.

Anne

comment:75 Changed 6 years ago by git

  • Commit changed from c6d57ec0a5207d39d08ae01f6b8555623cea9927 to 1de1304480a40d8db891b4adfcb806606dc805f5

Branch pushed to git repo; I updated commit sha1. New commits:

1de1304Marked long tests and some doc formatting.

comment:76 Changed 6 years ago by tscrim

I've double-checked that all doctests passed. I marked one of them as # long time and fixed a doc formatting issue that I saw. However I can't verify the doc builds because my docbuild is (still) broken...

comment:77 Changed 6 years ago by git

  • Commit changed from 1de1304480a40d8db891b4adfcb806606dc805f5 to 8342eb7dd83362d21cba01ec9dac7fec47d7c5fd

Branch pushed to git repo; I updated commit sha1. New commits:

8342eb714102: Proofread latest commits by Travis

comment:78 follow-up: Changed 6 years ago by nthiery

Hi Anne, Travis,

Thanks for merging in the latest dependencies! I have proofread the changes, and am happy with them, up to one point: in FiniteEnumeratedSets? I'd rather stick to the lazy import. The methods are potencially called repeteadly and reimporting Integer each time could waste time, whereas the lazy import should be resolved only once. I haven't done a benchmark though.

Once this detail is settled, we can go back to positive review!

Cheers,

Nicolas

comment:79 in reply to: ↑ 78 ; follow-up: Changed 6 years ago by aschilling

Hi Nicolas,

Please change the import in FiniteEnumeratedSets? (Travis changed this, right?).

Thanks!

Anne

Replying to nthiery:

Hi Anne, Travis,

Thanks for merging in the latest dependencies! I have proofread the changes, and am happy with them, up to one point: in FiniteEnumeratedSets? I'd rather stick to the lazy import. The methods are potencially called repeteadly and reimporting Integer each time could waste time, whereas the lazy import should be resolved only once. I haven't done a benchmark though.

Once this detail is settled, we can go back to positive review!

Cheers,

Nicolas

comment:80 Changed 6 years ago by git

  • Commit changed from 8342eb7dd83362d21cba01ec9dac7fec47d7c5fd to 7285efe502514618c4e04080de2056c91933a062

Branch pushed to git repo; I updated commit sha1. New commits:

7285efe#14102: reverted a chunk of commit 41768a: lazy import there is slightly preferable

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

Replying to aschilling:

Please change the import in FiniteEnumeratedSets? (Travis changed this, right?).

Done!

For the record, here is the result of a quick benchmark I made out of curiosity. For a trivial function, we get a factor of 10 in speed. Of course it's not really representative since in our current use case the methods are not quite as trivial.

"""
Comparative benchmark between lazy importing and importing inside a method

    sage: from test_lazy_import import f,g
    sage: %timeit f(i)
    1000000 loops, best of 3: 197 ns per loop
    sage: %timeit g(i)
    1000000 loops, best of 3: 1.65 µs per loop
"""
from sage.misc.lazy_import import lazy_import
lazy_import('sage.rings.integer', 'Integer', 'LazyInteger')

def f(i):
    return LazyInteger(i)

def g(i):
    from sage.rings.integer import Integer
    Integer(i)

Btw: it would be nice if pyflakes knew about lazy_import.

comment:82 Changed 6 years ago by nthiery

  • Status changed from needs_review to positive_review

comment:83 Changed 6 years ago by nthiery

  • Keywords ICERM2013 added

comment:84 Changed 6 years ago by tscrim

That's a good benchmark to know. Thanks Nicolas.

comment:85 Changed 6 years ago by tscrim

  • Milestone changed from sage-pending to sage-6.3

comment:86 Changed 6 years ago by git

  • Commit changed from 7285efe502514618c4e04080de2056c91933a062 to 0d766ab4711e54ece6912381f8d834a27af7e192
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. Last 10 new commits:

8da7522Trac 10963: degraded a bit a newly added cross-link to work around 'make doc-clean; make clean' failure
4ef8b27Merge branch 'public/ticket/10963-doc-distributive' of git://trac.sagemath.org/sage into 10963new
afb911ccorrected conflict resolution
82c8ee0Merge branch 'public/ticket/10963-doc-distributive' of trac.sagemath.org:sage into public/ticket/10963-doc-distributive
96c631fMerge branch 'develop' into categories/axioms-10963
b1a2aedTrac 10963: two typo fixes to let the pdf documentation compile
c16f18bMerge branch 'public/ticket/10963-doc-distributive' of trac.sagemath.org:sage into categories/axioms-10963
a38b62cMerge branch 'public/ticket/10963-doc-distributive' of trac.sagemath.org:sage into public/ticket/10963-doc-distributive
b162cfdMerge branch 'develop' into public/ticket/10963-doc-distributive
0d766abMerge branch 'public/combinat/nonsymmetric_macdonald-14102' of trac.sagemath.org:sage into public/combinat/nonsymmetric_macdonald-14102

comment:87 Changed 6 years ago by tscrim

  • Status changed from needs_review to positive_review

Trivial rebase over latest #10963.

comment:88 follow-up: Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work
  • Work issues set to doc-pdf

pdf docs don't build

comment:89 in reply to: ↑ 88 Changed 6 years ago by aschilling

Replying to vbraun:

pdf docs don't build

Is this related to this patch? I get the following error:

Overfull \hbox (27.79457pt too wide) in paragraph at lines 54386--54386
\T1/ptm/m/it/10 bose=False\T1/pcr/m/n/10 ) 
[597] [598] [599] [600] [601] [602] [603] [604] [605] [606]
Chapter 6.
[607] [608]
Underfull \hbox (badness 6380) in paragraph at lines 55235--55239
[]\T1/ptm/m/n/10 Aric Hag-berg, Dan Schult and Pieter Swart. Net-workX doc-u-me
n-ta-tion. [On-line] Avail-able:

Underfull \hbox (badness 10000) in paragraph at lines 55291--55295
[]\T1/ptm/m/n/10 Stephen P Bor-gatti. (1995). Cen-tral-ity and AIDS. [On-line] 
Avail-able:

Underfull \hbox (badness 7558) in paragraph at lines 55296--55301
[]\T1/ptm/m/n/10 Coen Bron and Joep Ker-bosch. (1973). Al-go-rithm 457: Find-in
g All Cliques of an

Underfull \hbox (badness 5667) in paragraph at lines 55296--55301
\T1/ptm/m/n/10 Undi-rected Graph. Com-mun. ACM. v 16. n 9. pages 575-577. ACM P
ress. [On-line] Avail-able:
[609]
Underfull \hbox (badness 10000) in paragraph at lines 55416--55420
[]\T1/ptm/m/n/10 Fifth An-nual Graph Draw-ing Con-test P. Eaded, J. Marks, P.Mu
tzel, S. North
[610] [611] [612]
Underfull \hbox (badness 10000) in paragraph at lines 55704--55709
[]\T1/ptm/m/n/10 Computing Tutte Poly-no-mi-als. Gary Hag-gard, David J. Pearce
 and Gor-don Royle.

Underfull \hbox (badness 10000) in paragraph at lines 55704--55709
\T1/ptm/m/n/10 In ACM Trans-ac-tions on Math-e-mat-i-cal Soft-ware, Vol-ume 37(
3), ar-ti-cle 24, 2010. Preprint:
[613] [614] [615] (./graphs.ind [616] [617] [618] [619] [620] [621] [622]
[623] [624] [625] [626] [627] [628] [629] [630] [631] [632] [633] [634]
[635]) [636] (./graphs.aux) )
(see the transcript file for additional information){/usr/local/texlive/2008/te
xmf-dist/fonts/enc/dvips/base/8r.enc}</usr/local/texlive/2008/texmf-dist/fonts/
type1/bluesky/cm/cmex10.pfb></usr/local/texlive/2008/texmf-dist/fonts/type1/blu
esky/cm/cmmi10.pfb></usr/local/texlive/2008/texmf-dist/fonts/type1/bluesky/cm/c
mmi5.pfb></usr/local/texlive/2008/texmf-dist/fonts/type1/bluesky/cm/cmmi7.pfb><
/usr/local/texlive/2008/texmf-dist/fonts/type1/bluesky/cm/cmr10.pfb></usr/local
/texlive/2008/texmf-dist/fonts/type1/bluesky/cm/cmr5.pfb></usr/local/texlive/20
08/texmf-dist/fonts/type1/bluesky/cm/cmr7.pfb></usr/local/texlive/2008/texmf-di
st/fonts/type1/bluesky/cm/cmsy10.pfb></usr/local/texlive/2008/texmf-dist/fonts/
type1/bluesky/cm/cmsy5.pfb></usr/local/texlive/2008/texmf-dist/fonts/type1/blue
sky/cm/cmsy7.pfb></usr/local/texlive/2008/texmf-dist/fonts/type1/bluesky/ams/ms
am10.pfb></usr/local/texlive/2008/texmf-dist/fonts/type1/bluesky/ams/msbm10.pfb
></usr/local/texlive/2008/texmf-dist/fonts/type1/urw/courier/ucrb8a.pfb></usr/l
ocal/texlive/2008/texmf-dist/fonts/type1/urw/courier/ucrr8a.pfb></usr/local/tex
live/2008/texmf-dist/fonts/type1/urw/courier/ucrro8a.pfb></usr/local/texlive/20
08/texmf-dist/fonts/type1/urw/helvetic/uhvb8a.pfb></usr/local/texlive/2008/texm
f-dist/fonts/type1/urw/helvetic/uhvbo8a.pfb></usr/local/texlive/2008/texmf-dist
/fonts/type1/urw/helvetic/uhvr8a.pfb></usr/local/texlive/2008/texmf-dist/fonts/
type1/urw/times/utmb8a.pfb></usr/local/texlive/2008/texmf-dist/fonts/type1/urw/
times/utmr8a.pfb></usr/local/texlive/2008/texmf-dist/fonts/type1/urw/times/utmr
i8a.pfb>
Output written on graphs.pdf (640 pages, 2603829 bytes).
Transcript written on graphs.log.
make: *** [doc-pdf] Error 1

But as far as I know this branch does not touch graphs.

comment:90 Changed 6 years ago by vbraun

The actual error is higher up. The end of the log just shows that the graphs part did build successfully.

comment:91 Changed 6 years ago by git

  • Commit changed from 0d766ab4711e54ece6912381f8d834a27af7e192 to a0ceb91854a6899d33001c984f709ad5758e8317

Branch pushed to git repo; I updated commit sha1. New commits:

a0ceb9114102: fixed pdf documentation compilation: citation keys shall not use '_'

comment:92 Changed 6 years ago by nthiery

  • Status changed from needs_work to needs_review

comment:93 Changed 6 years ago by aschilling

  • Status changed from needs_review to positive_review

comment:94 Changed 6 years ago by vbraun

  • Branch changed from public/combinat/nonsymmetric_macdonald-14102 to a0ceb91854a6899d33001c984f709ad5758e8317
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.