Kashiwara crystals and Littelmann path model #4031

felix-roehrich · 2024-08-20T14:03:34Z

I am currently working on realizing the LS path model in OSCAR. The main goal is to replicate and extend the functionality currently provided in the QuaGroup package, while making it more convenient to use.

Since the path model is a Kashiwara crystal, I also want to provide an abstract type AbstractCrystal, since many things can be defined using only the abstract structure. Finding proper names for the functions proves rather difficult, since they have no explicit names in literature, so I would like to gather some suggestions.

Currently I have the following:

$\tilde e_i(b), \tilde f_i(b)$ = ealpha(b::AbstractCrystalElem, i::Int), falpha(b::AbstractCrystalElem, i::Int). The naming here was taken from the QuaGroup package, since I couldn't find anything better.
$\epsilon_i(b), \varphi_i(b)$ = eps(b::AbstractCrystalElem, i::Int), phi(b::AbstractCrystalElem, i::Int). Since eps already existed I hijacked it, not very happy with current names.
$wt(b)$ = weight(b::AbstractCrystalElem)

lgoettgens

some random things I noticed while looking through this.

lgoettgens · 2024-08-20T14:32:54Z

experimental/LieAlgebras/src/LieAlgebras.jl

@@ -130,10 +133,14 @@ export coxeter_matrix
 export derived_algebra
 export dim_of_simple_module
 export dominant_character
+export dominant_path
+export ealpha, ealpha!


I don't think these names are particularly readable. What about something like e_alpha or maybe e_operator?

lgoettgens · 2024-08-20T14:33:45Z

experimental/LieAlgebras/src/PathModel.jl

+
+# AbstractCrystalElem required methods
+
+function Base.eps(b::AbstractCrystalElem)


please don't overload the julia base function that gives you the machine precision

lgoettgens · 2024-08-20T14:34:38Z

experimental/LieAlgebras/src/PathModel.jl

+end
+
+@doc raw"""
+    Apply the root operator $\tilde e_i$ to `b` in place and return the result.


Suggested change

Apply the root operator $\tilde e_i$ to `b` in place and return the result.

ealpha!(b::AbstractCrystalElem, i::Int)

Apply the root operator $\tilde e_i$ to `b` in place and return the result.

This and the next few docstrings are indented weirdly

lgoettgens · 2024-08-20T14:35:17Z

experimental/LieAlgebras/src/PathModel.jl

+# AbstractCrystal optional methods
+
+@doc raw"""
+    is_upper_normal(B::AbstractCrystal) -> bool


Suggested change

is_upper_normal(B::AbstractCrystal) -> bool

is_upper_normal(B::AbstractCrystal) -> Bool

types in julia are uppercase (more similar places below)

lgoettgens · 2024-08-20T14:36:25Z

experimental/LieAlgebras/src/PathModel.jl

+@doc raw"""
+    ealpha(b::AbstractCrystalElem, i::Vector{Int}, n::Vector{Int}) -> AbstractCrystalElem
+
+Return the result of applying the root operator $\tilde e_i$ to `b`.


n is missing from the docstring

lgoettgens · 2024-08-20T14:40:59Z

experimental/LieAlgebras/src/PathModel.jl

+  return s
+end
+
+# LS Path Model 


maybe split up the file into 1. abstract crystal stuff and 2. path models?

lgoettgens · 2024-08-20T14:43:40Z

Maybe @fingolfin or @ulthiel have some opinion on how to name the abstract crystal functions (see initial comment by felix), or can ping some people that might want to take part in this?

fingolfin · 2024-08-21T08:21:38Z

I have no idea at all what a "Kashiwara crystal" is, or an "abstract crystal". After some quick googling it doesn't seem to have any obvious connection to "usual" crystals such as studied in chemistry, and for which crystallographic groups are made, so I don't feel qualified to comment.

thofma · 2024-08-21T08:33:27Z

I have no idea at all what a "Kashiwara crystal" is, or an "abstract crystal". After some quick googling it doesn't seem to have any obvious connection to "usual" crystals such as studied in chemistry, and for which crystallographic groups are made, so I don't feel qualified to comment.

Maybe this is a good reason to not call this AbstractCrystal.

Edit: There are also crystals in connection with crystalline cohomology.

fingolfin · 2024-08-20T15:54:23Z

experimental/LieAlgebras/docs/src/crystals.md

@@ -0,0 +1,19 @@
+# Crystals
+
+According to Kashiwara a crystal is a set $B$ together with the following maps


What are "the following maps" ?

fingolfin · 2024-08-21T08:21:52Z

experimental/LieAlgebras/docs/src/crystals.md

+
+According to Kashiwara a crystal is a set $B$ together with the following maps
+
+This is realised in OSCAR through the `AbstractCrystal` and `AbstractCrystalElem` interfaces and the following methods


This sentence should end with a period or colon.

fingolfin · 2024-08-21T08:22:10Z

experimental/LieAlgebras/docs/src/crystals.md

+weight(::AbstractCrystalElem)
+```
+
+Additionally the following methods are provided


This sentence should end with a period or colon.

fingolfin · 2024-08-21T08:23:20Z

experimental/LieAlgebras/src/LieAlgebras.jl


+export AbstractCrystal, AbstractCrystalElem


off-topic question for @lgoettgens: these export statements all have to be duplicate. Would it make sense to put them into an exports.jl and then just include that twice, in both locations?

That's a great idea. I just checked, and the two lists (already on current master), differ by some things (some names get exported just into the Oscar namespace but not exported from Oscar, and thus make it easier for other devs to use them and thus work against #3408 by accident). I'll prepare a PR for streamlining the two lists and putting them somewhere separate. This PR can then get rebased afterwards.

fingolfin · 2024-08-21T08:25:26Z

experimental/LieAlgebras/src/PathModel.jl

+@doc raw"""
+    is_upper_normal(B::AbstractCrystal) -> bool
+
+Return `true` if `B` is upper normal, and `false` if `B` is not upper normal or it is unknown.


Is this a standard term, that everyone working on these things would immediately understand? If not perhaps it could be briefly defined here?

Alternatively resp. in addition, a reference should be added to the bibliography that defines all these terms, and your .md file should point out this reference: "We follow the terminology and definitions in CITE".

fingolfin · 2024-08-21T08:28:57Z

experimental/LieAlgebras/src/PathModel.jl

+@doc raw"""
+    ealpha(b::AbstractCrystalElem, i::Int, n::Int) -> AbstractCrystalElem
+
+Return the result of applying the root operator $\tilde e_i$ to `b` `n`-times.


I would recommend to avoid putting formulas directly adjacent. How about this (and of course similarly in other places that do this):

Suggested change

Return the result of applying the root operator $\tilde e_i$ to `b` `n`-times.

Return the result of `n`-times applying the root operator $\tilde e_i$ to `b`.

(There is probably a more elegant way to phrase this, perhaps our native speakers @JohnAAbbott or @mjrodgers have a suggestion)

I think the original is good, but without the hyphen:
Return the result of applying the root operator to b n times.

Or alternately
Applies the root operator to b n times.
Applies the root operator n times to b.

Edit: Looks better maybe with the b and n separated.

Possibly: result of n-fold application of the root operator to b.
For readability we do need to keep b and n separated (by more than just a space).

fingolfin · 2024-08-21T08:29:32Z

experimental/LieAlgebras/src/PathModel.jl

+
+# other
+
+function adapted_string(p::AbstractCrystalElem, rdec::Vector{Int})


This is undocumented, and so looks like an internal helper. So why is it exported?

fingolfin · 2024-08-21T08:29:45Z

experimental/LieAlgebras/src/PathModel.jl

+
+# LS Path Model 
+
+struct LSPathModel


Please add a docstring

fingolfin · 2024-08-21T08:31:11Z

experimental/LieAlgebras/src/PathModel.jl

+  w::WeylGroupElem
+end
+
+function Base.:(==)(s1::LSPathSegment, s2::LSPathSegment)


When defining a custom == method, make sure to also (two argument) hash method if there is even a tiny chance of anyone ever trying to use these as keys in a dictionary or apply unique to them etc.

Seems you have one for LSPathModelElem, but not for LSPathSegment; and the former ends up hashing elements of the latter type, so I guess you really need this.

fingolfin · 2024-08-21T08:33:58Z

experimental/LieAlgebras/src/WeylGroup.jl

  rk = rank(root_system(parent(iter.el)))
+  if isempty(word)
+    return nothing
+  end


Suggested change

rk = rank(root_system(parent(iter.el)))

if isempty(word)

return nothing

end

isempty(word) && return nothing

rk = rank(root_system(parent(iter.el)))

This change is already included in #4030, so it can be dropped from this PR once #4030 is merged.

Suggested change

rk = rank(root_system(parent(iter.el)))

if isempty(word)

return nothing

end

rk = rank(root_system(parent(iter.el)))

#4030 is merged now

felix-roehrich · 2024-08-21T10:06:24Z

Maybe this is a good reason to not call this AbstractCrystal.

Edit: There are also crystals in connection with crystalline cohomology.

There is a difference between a Kashiwara crystal and an (abstract) crystal in the sense of Kashiwara, where the first one is a very specific crystal and the second came from abstracting the first. That's why naming it KashiwaraCrystal is bad, maybe AbstractKashiwaraCrystal is good enough to avoid confusion (even if KashiwaraCrystal is added at some point).

For the same reason naming falpha instead kashiwara_f is bad.

lgoettgens reviewed Aug 20, 2024

View reviewed changes

lgoettgens added the experimental Only changes experimental parts of the code label Aug 20, 2024

fingolfin reviewed Aug 21, 2024

View reviewed changes

lgoettgens mentioned this pull request Aug 21, 2024

LieAlgebras: Streamline exports in Oscar.LieAlgebras #4036

Merged

felix-roehrich force-pushed the fr/path-model branch from 1ebb681 to a21e7de Compare August 23, 2024 14:58

felix-roehrich added 4 commits September 18, 2024 11:58

KashiwaraCrystal and Path Model

1c79c35

revert experimental/LieAlgebras/src/LieAlgebras.jl

76c1a85

Move types into Types.jl

b9edc0a

use cartan_matrix_inv

6ecb671

felix-roehrich force-pushed the fr/path-model branch from 2569509 to 6ecb671 Compare September 18, 2024 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kashiwara crystals and Littelmann path model #4031

Kashiwara crystals and Littelmann path model #4031

felix-roehrich commented Aug 20, 2024

lgoettgens left a comment

lgoettgens Aug 20, 2024

lgoettgens Aug 20, 2024

lgoettgens Aug 20, 2024

lgoettgens Aug 20, 2024

lgoettgens Aug 20, 2024

lgoettgens Aug 20, 2024

lgoettgens commented Aug 20, 2024

fingolfin commented Aug 21, 2024

thofma commented Aug 21, 2024 •

edited

Loading

fingolfin Aug 20, 2024

fingolfin Aug 21, 2024

fingolfin Aug 21, 2024

fingolfin Aug 21, 2024

lgoettgens Aug 21, 2024

fingolfin Aug 21, 2024

fingolfin Aug 21, 2024

mjrodgers Aug 21, 2024 •

edited

Loading

JohnAAbbott Aug 21, 2024

fingolfin Aug 21, 2024

fingolfin Aug 21, 2024

fingolfin Aug 21, 2024

fingolfin Aug 21, 2024

lgoettgens Aug 21, 2024

lgoettgens Aug 21, 2024

felix-roehrich commented Aug 21, 2024


		# AbstractCrystalElem required methods

		function Base.eps(b::AbstractCrystalElem)

	is_upper_normal(B::AbstractCrystal) -> bool
	is_upper_normal(B::AbstractCrystal) -> Bool

		@@ -0,0 +1,19 @@
		# Crystals

		According to Kashiwara a crystal is a set $B$ together with the following maps


		According to Kashiwara a crystal is a set $B$ together with the following maps

		This is realised in OSCAR through the `AbstractCrystal` and `AbstractCrystalElem` interfaces and the following methods

	Return the result of applying the root operator $\tilde e_i$ to `b` `n`-times.
	Return the result of `n`-times applying the root operator $\tilde e_i$ to `b`.


		# other

		function adapted_string(p::AbstractCrystalElem, rdec::Vector{Int})

Kashiwara crystals and Littelmann path model #4031

Are you sure you want to change the base?

Kashiwara crystals and Littelmann path model #4031

Conversation

felix-roehrich commented Aug 20, 2024

lgoettgens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgoettgens commented Aug 20, 2024

fingolfin commented Aug 21, 2024

thofma commented Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjrodgers Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felix-roehrich commented Aug 21, 2024

thofma commented Aug 21, 2024 •

edited

Loading

mjrodgers Aug 21, 2024 •

edited

Loading