Opened 8 years ago

Closed 6 years ago

Last modified 5 years ago

#6588 closed enhancement (fixed)

Categories for root systems and many misc improvements

Reported by: nthiery Owned by: mhansen
Priority: major Milestone: sage-5.0
Component: combinatorics Keywords: root systems, categories
Cc: sage-combinat, mshimo@… Merged in: sage-5.0.beta10
Authors: Nicolas M. Thiéry Reviewers: Anne Schilling, Mark Shimozono, Florent Hivert
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #10817 Stopgaps:

Description (last modified by nthiery)

New Features:

- Extended weight lattice/space for affine types
- coxeter_matrix and coxeter_diagram (for crystallographic types)
- Embeddings of the root lattice/space in root lattice realizations
- Embeddings of the weight lattice/space in weight lattice realizations
- Partial conversion from the root space to the root lattice
- Iterator for any Coxeter group, and weak order ideals thereof
- Facade option for weak_poset's and bruhat_poset's of finite coxeter groups
- weak_poset's of finite coxeter groups are lattices; added link weak_lattice -> weak_poset
  (courtesy of Christian Stump)
- Enumerated set for grassmannian elements, and is_grasmmannian test
- Added method register_as_conversion to morphisms

Refactoring:

- RootLatticeRealization and WeightLatticeRealization are now categories
- Use abstract_methods where appropriate
- Cleanup of an_element
- to_coroot_lattice_morphism: renamed to to_coroot_lattice_space, and
  moved from RootLatticeRealization to RootSpace (it does not make
  sense for, e.g., the weight lattice).
- Moved the implementation of associated_coroot in root_lattice_realization to root_space where it belongs

Doc, tests and bug fixes:

- 100% doctests on sage.combinat.root_system, except for weyl_group and weyl_characters
- 100% doctests on CoxeterGroups
- Add most modules in sage.combinat.root_system to the reference manual and improved the quickref
- Misc doc fixes in the above modules
- Add minimal documentation to AffineWeylGroups
- Added systematic tests for associated_coroot
- Fixed further missing features revealed by this test:
  - reducible Cartan types were not seen as finite/simply laced/crystallographic as appropriate
  - symmetrizer needed to be generalized to reducible cartan types (including D2)
  - the relabelling for reducible cartan types was broken for affine types
- More tests for scalar products
- Added systematic consistency check between the simple_root and simple_roots methods
- same thing for fundamental_weight and fundamental_weights
- Fixed a SL vs GL glitch when obtaining simple roots for reducible cartan types revealed by the above
- Fixed imports (extraneous ones, and a missing one reported by M. Shimozono and others)

Apply: trac_6588-categories-root_systems-nt.patch

Attachments (3)

trac_6588-categories-root_systems-nt-before-reindentation.patch (161.7 KB) - added by nthiery 6 years ago.
This contains the user-readable diff. Do not apply!
latest_change.patch (5.3 KB) - added by nthiery 6 years ago.
trac_6588-categories-root_systems-nt.patch (312.7 KB) - added by jdemeyer 6 years ago.
Apply this one (identical to the other, but renames and reindents two files; thus unreadable)

Download all attachments as: .zip

Change History (51)

comment:1 Changed 6 years ago by nthiery

  • Authors set to Nicolas M. Thiéry
  • Cc mshimo@… added
  • Dependencies set to #10963, #10817
  • Description modified (diff)
  • Report Upstream set to N/A

comment:2 Changed 6 years ago by nthiery

  • Description modified (diff)
  • Summary changed from Root systems: categorification to Categories for root systems

comment:3 Changed 6 years ago by nthiery

  • Description modified (diff)

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

  • Status changed from new to needs_review

The updated patch adds many missing doctests, and improves coxeter diagrams. It's probably close to completion, up to fixing some potentially failing doctests.

comment:5 Changed 6 years ago by nthiery

Beside, I commuted it up the Sage-Combinat queue. In principle, there should not be other dependencies than the listed ones.

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

Replying to nthiery:

The updated patch adds many missing doctests, and improves coxeter diagrams. It's probably close to completion, up to fixing some potentially failing doctests.

There are indeed a couple minor ones:

http://sage.math.washington.edu/home/nthiery/trac_6588-categories-root_systems-nt.patch-testlog

Note: that's with the following patches applied on 5.0.beta5:

trac_12476-lattice_join_matrix_speedup-fh.patch
trac_9469-category-membership_without_arguments-nt.patch
trac_10603-union_enumset_elconstr_fix-fh.patch
trac_12528_free_module-optimize-nt.patch
trac_10817-generalized_associahedra-cs.patch
trac_10817-generalized_associahedra-review-nt.patch
trac_12078-see_also-fh.patch
trac_9128-intersphinx_python_database-fh.patch
trac_9128-sphinx_links_all-fh.patch
trac_12527-fraction_field-is_exact_optimization-nt.patch
trac_12510-nonzero_equal_consistency-fh.patch
trac_12536-linear_extensions-as.patch
trac_12518-enumerated_set_from_iterator-vd.patch
trac_11880.patch
trac_11880-graph_classes-review-nt.patch
trac_7980-multiple-realizations-nt.patch
trac_7980-multiple-realizations-review-nt.patch
trac_6588-categories-root_systems-nt.patch

Off for skiing :-)

comment:7 Changed 6 years ago by nthiery

I fixed the doctests failures due to this patch. Most of them where due to the fact that affine weyl groups are iterable now, which gives a nicer an_element on any free module thereupon.

The other doctests failures showing up in the log are related to #12518 (or to the ISGCI patch, but that's because I did not upload the appropriate database on my test server).

The review can start!

comment:8 Changed 6 years ago by nthiery

  • Dependencies changed from #10963, #10817 to #10817
  • Description modified (diff)

Patch updated after a bug report by Mark; see end of patch description

comment:9 Changed 6 years ago by nthiery

Upon popular demand, extended weight lattice/spaces are now implemented.

comment:10 follow-up: Changed 6 years ago by aschilling

  • Reviewers set to Anne Schilling, Mark Shimozono

Hi Nicolas,

Impressive patch! Thanks for working on this. Here are some first comments:

  • In /sage/categories/affine_weyl_groups.py there is a new import

from sage.categories.infinite_enumerated_sets import InfiniteEnumeratedSets? which is not used. Please either remove this line or add InfiniteEnumeratedSets? in the class (which is probably preferable).

  • In /sage/categories/affine_weyl_groups.py please add a TestSuite(s).run() doctest.
  • I am not sure this is related to the patch, but there are some

strange methods in /sage/categories/coxeter_groups.py without doctests:

@abstract_method(optional = True) def has_right_descent(self, i):

""" Returns whether i is a right descent of self.

# EXAMPLES:: # # sage:

"""

def has_left_descent(self, i):

""" Returns whether i is a left descent of self.

This default implementation uses that a left descent of w is a right descent of w^{-1}. """ return (~self).has_right_descent(i)

Should has_left_descent also be an abstract_method? Or is that implicit through has_right_descent?

/sage/combinat/root_system/root_lattice_realization.py here and not in categories (if it is a category as specified in the docstring)?

The same question holds for WeightLatticeRealizations?(Category_over_base_ring) in /sage/combinat/root_system/weight_lattice_realization.py.

  • When using the extended weight lattice, the list of fundamental weights

does not include \delta. On the other hand it is possible to input \delta into the method fundamental_weight. This seems a little inconsistent.

    sage: Q = RootSystem(['A', 3, 1]).weight_lattice(extended = True); Q
    Extended weight lattice of the Root system of type ['A', 3, 1]
    sage: Q.fundamental_weights()
    Finite family {0: Lambda[0], 1: Lambda[1], 2: Lambda[2], 3: Lambda[3]}
    sage: Q.fundamental_weight('delta')
    delta

Also, I posted a first reviewer's patch on sage-combinat with mostly just trivial changes. Please fold it if you are satisfied.

Thanks!

Anne

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

Thanks much Anne for your detailed review!

Replying to aschilling:

  • In /sage/categories/affine_weyl_groups.py there is a new import

from sage.categories.infinite_enumerated_sets import InfiniteEnumeratedSets? which is not used. Please either remove this line or add InfiniteEnumeratedSets? in the class (which is probably preferable).

Good point. Done!

  • In /sage/categories/affine_weyl_groups.py please add a TestSuite(s).run() doctest.

Done.

  • I am not sure this is related to the patch, but there are some

strange methods in /sage/categories/coxeter_groups.py without doctests:

CoxeterGroups? is now 100% doctested.

Should has_left_descent also be an abstract_method? Or is that implicit through has_right_descent?

There is a default implementation for this method, so it's not abstract. Indeed, it would be nice to track such dependencies in the documentation; but we don't have the infrastructure for that.

/sage/combinat/root_system/root_lattice_realization.py here and not in categories (if it is a category as specified in the docstring)?

The same question holds for WeightLatticeRealizations?(Category_over_base_ring) in /sage/combinat/root_system/weight_lattice_realization.py.

  • When using the extended weight lattice, the list of fundamental weights

does not include \delta. On the other hand it is possible to input \delta into the method fundamental_weight. This seems a little inconsistent.

    sage: Q = RootSystem(['A', 3, 1]).weight_lattice(extended = True); Q
    Extended weight lattice of the Root system of type ['A', 3, 1]
    sage: Q.fundamental_weights()
    Finite family {0: Lambda[0], 1: Lambda[1], 2: Lambda[2], 3: Lambda[3]}
    sage: Q.fundamental_weight('delta')
    delta

More on those after lunch!

Also, I posted a first reviewer's patch on sage-combinat with mostly just trivial changes. Please fold it if you are satisfied.

Folded in! There is a new reviewer's patch on the patch server.

Cheers,

Nicolas

comment:12 in reply to: ↑ 11 ; follow-up: Changed 6 years ago by nthiery

/sage/combinat/root_system/root_lattice_realization.py here and not in categories (if it is a category as specified in the docstring)?

The same question holds for WeightLatticeRealizations?(Category_over_base_ring) in /sage/combinat/root_system/weight_lattice_realization.py.

Yeah, we had a similar discussion for the crystal categories and the categories for Symmetric Functions and friends. And there is a non trivial borderline to set.

On one hand, we have general categories (like Groups, Algebras, ...) which are likely to get used in several Sage modules. Also, they name a well-known area of mathematics; so it's natural to import them by default in the interpreter, if not just as an entry point.

On the other hand we have categories that are rather specific to a Sage module, if not just implementation details. So it's good to keep them in this module, in particular to be as close as close as possible from the other sources of that module.

CoxeterGroups? clearly fits the first case (most mathematicians have heard of them; this category is used by WeylGroup? (in sage.combinat.root_system) and by SymmetricGroup? (in sage.groups).

The categories for symmetric functions are basically implementation details and fits in the second case. Anyway, SymmetricFunctions? is already a good entry point.

Crystals are only implemented in sage.combinat.crystals (so far). But this names an area of mathematics. So this fits a bit more the first case.

RootLatticeRealizations? and WeightLatticeRealizations? seem rather specific. So this fits a bit more the second case.

  • When using the extended weight lattice, the list of fundamental weights

does not include \delta. On the other hand it is possible to input \delta into the method fundamental_weight. This seems a little inconsistent.

Yeah, this abuse is documented in "fundamental_weight". Basically, we need a function that describes the embedding of the basis elements of the (extended) weight lattice. That's almost what fundamental_weight does, and I did not have a good alternative name. So I abused fundamental_weight to do just a bit more that its natural job. Anyway, we currently only have a single implementation of WeightLatticeRealizations? in the affine case, so it's very localized, and easy to change in the future if we come up with a good name (unless you have one!).

Let me know if you are ok with the above and with my reviewer's patch. If yes, I'll fold the patches, reindent properly root_lattice_realization.py and weight_lattice_realization.py, and add a 's' at the end of those files (I haven't done those yet to keep the diff meaningful). Then, the patch will be good to go.

For the record, all tests pass on Sage 5.0 beta6, with the following patches applied:

trac_10817-generalized_associahedra-cs.patch
trac_12645-fix_rst_markup-sk.patch
trac_9128-intersphinx_python_database-fh.patch
trac_9128-sphinx_links_all-fh.patch
trac_9128-MANIFEST-fh.patch
trac_12527-fraction_field-is_exact_optimization-nt.patch
trac_12510-nonzero_equal_consistency-fh.patch
trac_12536-linear_extensions-as.patch
element_compare_consistency-fh.patch
trac_11880-isgci-all_in_one-nc.patch
trac_11880-isgci-more-review-nt.patch
trac_7980-multiple-realizations-nt.patch
trac_7980-multiple-realizations-review-nt.patch
trac_6588-categories-root_systems-nt.patch
trac_6588-categories-root_systems-review-nt.patch

Cheers,

Nicolas

comment:13 follow-up: Changed 6 years ago by davidloeffler

  • Reviewers changed from Anne Schilling, Mark Shimozono to Anne Schilling, Mark Shimozono, PatchBot
  • Status changed from needs_review to needs_work

Fails doctests on Sage 5.0.beta7:

sage -t -long devel/sage-main/sage/combinat/root_system/weight_space.py
**********************************************************************
File "/storage/masiao/sage-5.0.beta7/devel/sage-main/sage/combinat/root_system/weight_space.py", line 126:
    sage: Q.null_root(0)[index]
Exception raised:
    Traceback (most recent call last):
      File "/storage/masiao/sage-5.0.beta7/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/storage/masiao/sage-5.0.beta7/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/storage/masiao/sage-5.0.beta7/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_1[17]>", line 1, in <module>
        Q.null_root(Integer(0))[index]###line 126:
    sage: Q.null_root(0)[index]
    TypeError: __call__() takes exactly 0 positional arguments (1 given)
**********************************************************************

(The patchbot found this, but I also reproduced it by hand)

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

  • Status changed from needs_work to needs_review

Replying to davidloeffler:

Fails doctests on Sage 5.0.beta7:

sage -t -long devel/sage-main/sage/combinat/root_system/weight_space.py
**********************************************************************
File "/storage/masiao/sage-5.0.beta7/devel/sage-main/sage/combinat/root_system/weight_space.py", line 126:
    sage: Q.null_root(0)[index]
Exception raised:
    Traceback (most recent call last):
      File "/storage/masiao/sage-5.0.beta7/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/storage/masiao/sage-5.0.beta7/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/storage/masiao/sage-5.0.beta7/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_1[17]>", line 1, in <module>
        Q.null_root(Integer(0))[index]###line 126:
    sage: Q.null_root(0)[index]
    TypeError: __call__() takes exactly 0 positional arguments (1 given)
**********************************************************************

(The patchbot found this, but I also reproduced it by hand)

Sorry, already fixed on the Sage-Combinat queue where the review is occuring. I'll delete the patch here for the moment so that others don't waste time reviewing an outdated version.

Thanks though for the report!

comment:15 Changed 6 years ago by nthiery

  • Description modified (diff)

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

Replying to nthiery:

Thanks much Anne for your detailed review!

You are welcome. I hope you will return the favor for trac_12536-linear_extensions-as.patch!

/sage/combinat/root_system/root_lattice_realization.py here and not in categories (if it is a category as specified in the docstring)?

The same question holds for WeightLatticeRealizations?(Category_over_base_ring) in /sage/combinat/root_system/weight_lattice_realization.py.

Yeah, we had a similar discussion for the crystal categories and the categories for Symmetric Functions and friends. And there is a non trivial borderline to set.

On one hand, we have general categories (like Groups, Algebras, ...) which are likely to get used in several Sage modules. Also, they name a well-known area of mathematics; so it's natural to import them by default in the interpreter, if not just as an entry point.

On the other hand we have categories that are rather specific to a Sage module, if not just implementation details. So it's good to keep them in this module, in particular to be as close as close as possible from the other sources of that module.

CoxeterGroups? clearly fits the first case (most mathematicians have heard of them; this category is used by WeylGroup? (in sage.combinat.root_system) and by SymmetricGroup? (in sage.groups).

The categories for symmetric functions are basically implementation details and fits in the second case. Anyway, SymmetricFunctions? is already a good entry point.

Crystals are only implemented in sage.combinat.crystals (so far). But this names an area of mathematics. So this fits a bit more the first case.

No, there is a lot of crystal code in /sage/categories: crystals, finite_crystals, highest_weight_crystals, ....

  • When using the extended weight lattice, the list of fundamental weights

does not include \delta. On the other hand it is possible to input \delta into the method fundamental_weight. This seems a little inconsistent.

Yeah, this abuse is documented in "fundamental_weight". Basically, we need a function that describes the embedding of the basis elements of the (extended) weight lattice. That's almost what fundamental_weight does, and I did not have a good alternative name. So I abused fundamental_weight to do just a bit more that its natural job. Anyway, we currently only have a single implementation of WeightLatticeRealizations? in the affine case, so it's very localized, and easy to change in the future if we come up with a good name (unless you have one!).

Why can't fundamental_weights when "extended" is set also include delta? This would at least fit with the notational abuse of fundamental_weight.

Let me know if you are ok with the above and with my reviewer's patch. If yes, I'll fold the patches, reindent properly root_lattice_realization.py and weight_lattice_realization.py, and add a 's' at the end of those files (I haven't done those yet to keep the diff meaningful). Then, the patch will be good to go.

I also wrote another very small reviewer's patch that you can fold in.

For the record, all tests pass on Sage 5.0 beta6, with the following patches applied:

trac_10817-generalized_associahedra-cs.patch
trac_12645-fix_rst_markup-sk.patch
trac_9128-intersphinx_python_database-fh.patch
trac_9128-sphinx_links_all-fh.patch
trac_9128-MANIFEST-fh.patch
trac_12527-fraction_field-is_exact_optimization-nt.patch
trac_12510-nonzero_equal_consistency-fh.patch
trac_12536-linear_extensions-as.patch
element_compare_consistency-fh.patch
trac_11880-isgci-all_in_one-nc.patch
trac_11880-isgci-more-review-nt.patch
trac_7980-multiple-realizations-nt.patch
trac_7980-multiple-realizations-review-nt.patch
trac_6588-categories-root_systems-nt.patch
trac_6588-categories-root_systems-review-nt.patch

That's good!

Cheers,

Anne

comment:17 in reply to: ↑ 16 ; follow-up: Changed 6 years ago by nthiery

Replying to aschilling:

You are welcome. I hope you will return the favor for trac_12536-linear_extensions-as.patch!

I sure will, soon!

Crystals are only implemented in sage.combinat.crystals (so far). But this names an area of mathematics. So this fits a bit more the first case.

No, there is a lot of crystal code in /sage/categories: crystals, finite_crystals, highest_weight_crystals, ....

I meant: the crystals themselves (CrystalOfLetter?, KR, ...) are all in implemented in sage.combinat.crystals.

Why can't fundamental_weights when "extended" is set also include delta? This would at least fit with the notational abuse of fundamental_weight.

Because we want to do things like:

sage: x in self.fundamental_weights()

sage: all(L.some_property() for L in self.fundamental_weights())

So adding delta would change the semantic of fundamental_weights. Whereas the current abuse of fundamental_weight can't break any code.

I also wrote another very small reviewer's patch that you can fold in.

I don't see it on the queue; did you push?

Thanks!

Nicolas

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

Replying to nthiery:

Replying to aschilling:

You are welcome. I hope you will return the favor for trac_12536-linear_extensions-as.patch!

I sure will, soon!

Thanks.

Crystals are only implemented in sage.combinat.crystals (so far). But this names an area of mathematics. So this fits a bit more the first case.

No, there is a lot of crystal code in /sage/categories: crystals, finite_crystals, highest_weight_crystals, ....

I meant: the crystals themselves (CrystalOfLetter?, KR, ...) are all in implemented in sage.combinat.crystals.

But these are not categories, they are classes. In your case, a cateogory is in /sage/combinat/root_systems/

Why can't fundamental_weights when "extended" is set also include delta? This would at least fit with the notational abuse of fundamental_weight.

Because we want to do things like:

sage: x in self.fundamental_weights()

sage: all(L.some_property() for L in self.fundamental_weights())

So adding delta would change the semantic of fundamental_weights. Whereas the current abuse of fundamental_weight can't break any code.

How about self.fundamental_weights(extended = True) with default extended = False?

I also wrote another very small reviewer's patch that you can fold in.

I don't see it on the queue; did you push?

Sorry, I forgot to push. It should be there now.

Best,

Anne

Changed 6 years ago by nthiery

This contains the user-readable diff. Do not apply!

comment:19 Changed 6 years ago by nthiery

  • Description modified (diff)

comment:20 Changed 6 years ago by nthiery

  • Description modified (diff)
  • Summary changed from Categories for root systems to Categories for root systems and many misc improvements

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

Replying to aschilling:

But these are not categories, they are classes. In your case, a cateogory is in /sage/combinat/root_systems/

Yes: unlike for crystals, and like for symmetric functions (well for NCSF actually; the categorification of symmetric functions is yet to be done), I want to keep the categories close to the classes. As I said, it's borderline, but it feels better to me this way.

How about self.fundamental_weights(extended = True) with default extended = False?

Well, since it's an abuse, and one that we might want to get rid of, I'd rather not abuse yet another function. Unless you have a natural use case for this notation? (for the extended weight lattice, that's just self.basis()).

Sorry, I forgot to push.

Given how many times I played that gag to you, I am not going to throw the first rock :-)

It should be there now.

Yes, thanks! I have folded together all patches and posted them here. I also did the reindentation and renaming *_lattice_realization -> *_lattice_realizations. On my side, the patch is good to go.

Cheers,

Nicolas

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

  • Status changed from needs_review to positive_review

How about self.fundamental_weights(extended = True) with default extended = False?

Well, since it's an abuse, and one that we might want to get rid of, I'd rather not abuse yet another function. Unless you have a natural use case for this notation? (for the extended weight lattice, that's just self.basis()).

Ok, since this is in self.basis() I am satisfied.

Yes, thanks! I have folded together all patches and posted them here. I also did the reindentation and renaming *_lattice_realization -> *_lattice_realizations. On my side, the patch is good to go.

Ok, thanks! Unless I hear otherwise from Mark, I'll set a positive review.

Cheers,

Anne

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

Good morning Anne!

Replying to aschilling:

Ok, thanks! Unless I hear otherwise from Mark, I'll set a positive review.

Yippee!

A bunch of patches went into 5.0.beta8 this morning. Maybe this one will follow :-)

I am working on the review of #12536.

Cheers,

Nicolas

comment:24 follow-up: Changed 6 years ago by jdemeyer

  • Reviewers changed from Anne Schilling, Mark Shimozono, PatchBot to Anne Schilling, Mark Shimozono

Let's not anthropomorphize the PatchBot? :-)

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

Hi Nicolas,

When working on a review for #12667, I noticed some documentation problems with this patch in root_lattice_realizations. I wrote a review patch on the sage-combinat queue. If you are happy, please fold, repost here and reset the positive review.

Best,

Anne

comment:26 Changed 6 years ago by aschilling

  • Status changed from positive_review to needs_work

comment:27 Changed 6 years ago by nthiery

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

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

Replying to aschilling:

When working on a review for #12667, I noticed some documentation problems with this patch in root_lattice_realizations. I wrote a review patch on the sage-combinat queue. If you are happy, please fold, repost here and reset the positive review.

Good catches. Thanks! Folded.

I allowed myself to further add the weight lattice realizations file to the reference manual, and to replace {\vee} by \vee. I also set back \mathbb{N} to \NN, since \NN will be added very soon to the standard Sage macros.

comment:29 Changed 6 years ago by nthiery

While I was at it, I added all the other missing files in root_system.rst (type_*, ...), and fixed the documentation typos revealled by the compilation. Florent reviewed them. New patch posted.

comment:30 Changed 6 years ago by nthiery

  • Description modified (diff)

comment:31 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

comment:32 Changed 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review

I guess the last patch still needs review?

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

  • Reviewers changed from Anne Schilling, Mark Shimozono to Anne Schilling, Mark Shimozono, Florent Hivert
  • Status changed from needs_review to positive_review

No, I had uploaded the wrong file. The new one is the one that has been checked by Florent.

Now it's a very final positive review. Sorry Jeroen for the mess; I hope you were not in the process of applying / testing it!

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

I am not sure why you replaced \mathcal{N} by \NN since this currently gives a latex error when compiling the documentation:

preparing documents... done WARNING: display latex u'
NN': latex exited with error:lattice_realizations [stderr]

[stdout] This is pdfTeXk, Version 3.1415926-1.40.9 (Web2C 7.5.7)

%&-line parsing enabled.

entering extended mode (./math.tex LaTeX2e <2005/12/01> Babel <v3.8l> and hyphenation patterns for english, usenglishmax, dumylang, noh yphenation, german-x-2008-06-18, ngerman-x-2008-06-18, ancientgreek, ibycus, ar abic, basque, bulgarian, catalan, pinyin, coptic, croatian, czech, danish, dutc h, esperanto, estonian, farsi, finnish, french, galician, german, ngerman, mono greek, greek, hungarian, icelandic, indonesian, interlingua, irish, italian, la tin, lithuanian, mongolian, mongolian2a, bokmal, nynorsk, polish, portuguese, r omanian, russian, sanskrit, serbian, slovak, slovenian, spanish, swedish, turki sh, ukenglish, ukrainian, uppersorbian, welsh, loaded. (/usr/local/texlive/2008/texmf-dist/tex/latex/base/article.cls Document Class: article 2005/09/16 v1.4f Standard LaTeX document class (/usr/local/texlive/2008/texmf-dist/tex/latex/base/size12.clo)) (/usr/local/texlive/2008/texmf-dist/tex/latex/base/inputenc.sty (/usr/local/texlive/2008/texmf-dist/tex/latex/base/utf8.def (/usr/local/texlive/2008/texmf-dist/tex/latex/base/t1enc.dfu) (/usr/local/texlive/2008/texmf-dist/tex/latex/base/ot1enc.dfu) (/usr/local/texlive/2008/texmf-dist/tex/latex/base/omsenc.dfu))) (/usr/local/texlive/2008/texmf-dist/tex/latex/amsmath/amsmath.sty For additional information on amsmath, use the `?' option. (/usr/local/texlive/2008/texmf-dist/tex/latex/amsmath/amstext.sty (/usr/local/texlive/2008/texmf-dist/tex/latex/amsmath/amsgen.sty)) (/usr/local/texlive/2008/texmf-dist/tex/latex/amsmath/amsbsy.sty) (/usr/local/texlive/2008/texmf-dist/tex/latex/amsmath/amsopn.sty)) (/usr/local/texlive/2008/texmf-dist/tex/latex/amscls/amsthm.sty) (/usr/local/texlive/2008/texmf-dist/tex/latex/amsfonts/amssymb.sty (/usr/local/texlive/2008/texmf-dist/tex/latex/amsfonts/amsfonts.sty)) (/usr/local/texlive/2008/texmf-dist/tex/latex/tools/bm.sty) (./math.aux) (/usr/local/texlive/2008/texmf-dist/tex/latex/amsfonts/umsa.fd) (/usr/local/texlive/2008/texmf-dist/tex/latex/amsfonts/umsb.fd) ! Undefined control sequence. <recently read> \NN l.29 $\NN

$

[1] (./math.aux) ) (see the transcript file for additional information) Output written on math.dvi (1 page, 152 bytes). Transcript written on math.log.

Is it allowed to leave such a failure?

Thanks,

Anne

comment:35 in reply to: ↑ 34 ; follow-up: Changed 6 years ago by nthiery

Hi Anne:

Replying to aschilling:

I am not sure why you replaced \mathcal{N} by \NN since this currently gives a latex error when compiling the documentation:

As I said: I also set back \mathbb{N} to \NN, since \NN will be added very soon to the standard Sage macros

There are lots of other failures like this in the documentation; it's best to progressively get rid of them, but here it's just temporary, so that's good enough. Let's not waste time on having to update this later.

Cheers,

Nicolas

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

Hi Nicolas,

Well, one of my patches recently did not get merged until all these failures were rectified. So I thought it was compulsory for all documentation to work.

Best,

Anne

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

Replying to aschilling:

Well, one of my patches recently did not get merged until all these failures were rectified. So I thought it was compulsory for all documentation to work.

There is at least one other \NN in th sources. Anyway, let's not waste even more time discussing it. Here is the diff with the patch I am about to upload(sorry Jeroen):

-+                with the coroots are all in `\NN`, which is not
++                with the coroots are all non negative integers, which is not

I left the positive review; feel free to change.

comment:38 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This obviously fails on 32-bit systems:

sage -t  "devel/sage/sage/combinat/root_system/dynkin_diagram.py"
**********************************************************************
File "/export/home/buildbot/sage-5.0.beta9/devel/sage/sage/combinat/root_system/dynkin_diagram.py", line 167:
    sage: hash(CartanType(['A',3]).dynkin_diagram()) # indirect doctest
Expected:
    286820813001824631
Got:
    -2127980169
**********************************************************************

comment:39 Changed 6 years ago by nthiery

  • Status changed from needs_work to needs_review

Thanks Jeroen for the report. This should be fixed with the upcoming patch. The diff is:

  • sage/combinat/root_system/dynkin_diagram.py

    diff --git a/sage/combinat/root_system/dynkin_diagram.py b/sage/combinat/root_system/dynkin_diagram.py
    a b class DynkinDiagram_class(DiGraph, Carta 
    165165        EXAMPLES::
    166166
    167167            sage: hash(CartanType(['A',3]).dynkin_diagram()) # indirect doctest
    168             286820813001824631
    169 
     168            286820813001824631     # 64-bit
     169            -2127980169            # 32-bit
    170170        """
    171171        # Should assert for immutability!
    172172

comment:40 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  • Work issues set to documentation

There is a problem with the documentation. Doing "make doc" after applying this patch gives:

reading sources... [ 92%] sage/combinat/root_system/weight_lattice_realizations
reading sources... [ 95%] sage/combinat/root_system/weight_space
reading sources... [ 97%] sage/combinat/root_system/weyl_group
reading sources... [100%] sage/structure/parent

<autodoc>:0: ERROR: Inconsistent literal block quoting.
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [  2%] categories
writing output... [  4%] combinat/algebra
writing output... [  6%] combinat/index
writing output... [  8%] combinat/root_systems
writing output... [ 10%] index

Unfortunately, there is no indication which file caused the problem.

comment:41 Changed 6 years ago by nthiery

Ok, it was in root_lattice_realizations. At this occasion, Anne and myself spotted a couple more doc typos. I am about to upload an updated patch. See :attachment:latest_change.patch for the changes since last version.

Changed 6 years ago by nthiery

comment:42 Changed 6 years ago by aschilling

  • Status changed from needs_work to positive_review

comment:43 Changed 6 years ago by jdemeyer

  • Work issues documentation deleted

comment:44 in reply to: ↑ 37 Changed 6 years ago by hivert

There is at least one other \NN in th sources. Anyway, let's not waste even more time discussing it. Here is the diff with the patch I am about to upload(sorry Jeroen):

I finished my patch on \NN: this is #12717

Changed 6 years ago by jdemeyer

Apply this one (identical to the other, but renames and reindents two files; thus unreadable)

comment:45 Changed 6 years ago by jdemeyer

Rebased the last patch to sage-5.0.beta9 (there was some trivial fuzz).

comment:46 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.0.beta10
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:47 Changed 6 years ago by nthiery

Yippee! Thanks all for your help getting this in!

comment:48 Changed 5 years ago by jdemeyer

The examples for __cmp__ in sage/combinat/root_system/type_dual.py are bad. #12793 fixes them, please review.

Note: See TracTickets for help on using tickets.