#15567 closed defect (fixed)

Fix Alphabet and improvements to Family

Reported by: tscrim Owned by: sage-combinat
Priority: major Milestone: sage-6.2
Component: combinatorics Keywords: alphabet
Cc: sage-combinat, nthiery, vdelecroix Merged in:
Authors: Travis Scrimshaw Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: public/15567 (Commits) Commit: f73d436e2590d5a4134b813460464dd62267c6fe
Dependencies: Stopgaps:

Description

Currently Alphabet(oo, 'x') returns None. This fixes that, along with some other improvements so that Alphabet(NN, 'x') also works as expected. This also uses the name argument in (Lazy)Family to override the name of the function for cleaner print statements.

Change History (37)

comment:1 Changed 12 months ago by tscrim

  • Status changed from new to needs_review

comment:2 follow-up: Changed 12 months ago by ncohen

  • Status changed from needs_review to needs_work

Hello !

I don't know how this "names" argument is used at all, but its current behaviour is the following :

sage: build_alphabet(oo,'x')[4]                      
'x4'
sage: build_alphabet(range(40),'x')[4]
4

Which is not very consistent. While playing with it I found something else which totally scares me :

sage: build_alphabet(oo,'x')          
Lazy family (x(i))_{i in Non negative integers}
sage: build_alphabet(oo,'x')[:4]
'xslice(None, 4, None)'
sage: print list(build_alphabet(oo,'x')[:4])
['x', 's', 'l', 'i', 'c', 'e', '(', 'N', 'o', 'n', 'e', ',', ' ', '4', ',', ' ', 'N', 'o', 'n', 'e', ')']

While I expected to get the list of the first 4 elements. And I really do not know what to make with that O_o

Anyway, I think that the build_alphabet function could use a "INPUT" section explaining its parameters, and in particular the "names" field. Which, in the context of build_alphabet(oo,'x') would be better renamed to "prefix", as it adds this string to all members of the family.

It would be nice to say what exactlyl this function is supposed to return.

About the modifications to Family :

  • would it be possible to add to the documentation of "hidden_keys" an explanation of what exactly it does ? Does it mean that the hidden keys are "skipped", i.e. removed from "indices" ?
  • the explanation of what hidden_function is isn't very clear either O_o

The fact that the "name" argument does nothing for finite lists should be made clear I guess. I mean, what is it useful for exactly ? Lazy Families only ?

sage: Family([0,0,0],function=lambda x:x,name="e")
Finite family {0: 0}

By the way, perhaps there should be in Sage something called "named function" somewhere :-)

Nathann

comment:3 in reply to: ↑ 2 ; follow-ups: Changed 12 months ago by tscrim

Replying to ncohen:

Hello !

I don't know how this "names" argument is used at all, but its current behaviour is the following :

sage: build_alphabet(oo,'x')[4]                      
'x4'
sage: build_alphabet(range(40),'x')[4]
4

Which is not very consistent.

This is also inconsistent input types (one is a "number", the other is a set). However the other could raise an error as invalid input as opposed to being silently ignored.

While playing with it I found something else which totally scares me :

sage: build_alphabet(oo,'x')          
Lazy family (x(i))_{i in Non negative integers}
sage: build_alphabet(oo,'x')[:4]
'xslice(None, 4, None)'
sage: print list(build_alphabet(oo,'x')[:4])
['x', 's', 'l', 'i', 'c', 'e', '(', 'N', 'o', 'n', 'e', ',', ' ', '4', ',', ' ', 'N', 'o', 'n', 'e', ')']

While I expected to get the list of the first 4 elements. And I really do not know what to make with that O_o

This is an issue with Family in that it is directly passing a string representation of the input of __getitem__() without any safety checks (for speed). It's outside the scope of what I want to do in this ticket and has a landmine of "you've picked a specific ordering of the indices that may not be canonical".

Anyway, I think that the build_alphabet function could use a "INPUT" section explaining its parameters, and in particular the "names" field. Which, in the context of build_alphabet(oo,'x') would be better renamed to "prefix", as it adds this string to all members of the family.

I'll add it. Since "names" not (really) used anywhere else, I'll change it.

About the modifications to Family :

  • would it be possible to add to the documentation of "hidden_keys" an explanation of what exactly it does ? Does it mean that the hidden keys are "skipped", i.e. removed from "indices" ?

It means they are valid keys (indices) of the Family that are hidden.

  • the explanation of what hidden_function is isn't very clear either O_o

Then neither should be the one for function (or does it make sense if I change the a to a the in hidden_function?).

The fact that the "name" argument does nothing for finite lists should be made clear I guess. I mean, what is it useful for exactly?

So lazy familys display nicely (espically when using lambda functions).

comment:4 in reply to: ↑ 3 Changed 12 months ago by ncohen

Hellooooooooo !!

This is also inconsistent input types (one is a "number", the other is a set). However the other could raise an error as invalid input as opposed to being silently ignored.

Oh ? Well, if the second input is not supported so be it. Could you say in the docstring of Family what kind of input is expected, then ?

This is an issue with Family in that it is directly passing a string representation of the input of __getitem__() without any safety checks (for speed). It's outside the scope of what I want to do in this ticket and has a landmine of "you've picked a specific ordering of the indices that may not be canonical".

Well there has to be a bug somewhere, this container is returning something totally crazy when one uses a basic list operation on it O_o

I don't even understand how the string representation can get to replace the content itself O_o

It means they are valid keys (indices) of the Family that are hidden.

And what does hidden mean ? That they are skipped when one list the elements, or that they do not appear in the string representation __repr__ of the object ?... As it is, I don't get what this argument actually does.

So lazy familys display nicely (espically when using lambda functions).

Okay. Could you then say so in the docstring ? "name" only has an effect when the input defines a lambda function, if that's the explanation ?

Nathann

comment:5 in reply to: ↑ 3 Changed 12 months ago by vdelecroix

Replying to tscrim:

Since "names" not (really) used anywhere else, I'll change it.

Please keep "names" as an argument. I included it for future compatibility with the free monoid in sage.monoids.free_monoid (which should be merge with FiniteWords?). Nevertheless, I think I made it wrong as what I wanted was

sage: build_alphabet(3, ['a', 'b', 'c'])
{'a', 'b', 'c'}
sage: build_alphabet(3, 'x')
{'x0', 'x1', 'x2'}

and the first syntax currently does not work...

comment:6 Changed 12 months ago by tscrim

I haven't forgotten about this ticket, just been busy with other stuff. I'll get that to work Vincent on my next commit.

comment:7 Changed 11 months ago by git

  • Commit changed from b3396dd62e96a08621cf08a6b2573c59a664adac to aa8d6a95d8e24d828a96d6f5bf7f4c66e16bfbb1

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

d2d936aMerge branch 'develop' into public/combinat/words/fix_alphabet
3593bb4Merge branch 'develop' into public/combinat/words/fix_alphabet
6ca233dMerge branch 'develop' into public/combinat/words/fix_alphabet
8e3cbb1Merge branch 'develop' into public/combinat/words/fix_alphabet
aa8d6a9Expanded documentation for #15567.

comment:8 Changed 11 months ago by git

  • Commit changed from aa8d6a95d8e24d828a96d6f5bf7f4c66e16bfbb1 to aef8b9f94f928b0d457713aec118791b41ce1247

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

aef8b9fMore tweaks to alphabet construction for #15567.

comment:9 Changed 11 months ago by tscrim

  • Status changed from needs_work to needs_review

@Vincent: I've (unintentionally) made that work with this branch, but I will keep the names argument around.

@Nathann: I've made it so that names and data switch if necessary, as well as addressed some of your other concerns. I'm not going to fix the slices here (but I do agree it should be fixed, I'll post a fix to that as soon as I can). There's more detail and examples about the hidden arguments for Family further down in it's documentation. I've expanded upon the name argument documentation for Family.

comment:10 Changed 11 months ago by ncohen

Okay, these commits are getting ridiculous. Can I rewrite this branch's history and remove them ? O_O

comment:11 Changed 11 months ago by ncohen

I'm talking of the 4 'merge' commits O_o

Nathann

comment:12 Changed 11 months ago by ncohen

  • Branch changed from public/combinat/words/fix_alphabet to public/15567
  • Commit changed from aef8b9f94f928b0d457713aec118791b41ce1247 to 03b3045881fbff482768e4d3efee772c83d58da8

Okay I created another branch where I removed the 4 empty merges with every single beta release of Sage. If that is a problem we can switch back to public/combinat/words/fix_alphabet, but honestly let's keep the histoy simple O_o

Nathann


New commits:

051153cFixes to alphabet and now using name arg in Family.
ad53e23Expanded documentation for #15567.
03b3045More tweaks to alphabet construction for #15567.

comment:13 Changed 11 months ago by ncohen

  • Status changed from needs_review to needs_work

Hello.

I'm sorry but this doc has to be rewritten. What the hell does that even mean ?

Definition: Alphabet(data=None, names=None, name=None)
Docstring:
   Return an object representing an ordered alphabet.

   INPUT:

   * "data" -- the data to build the alphabet

   * "names" -- the letters, names, or prefix

   * "name" -- the name of a specific named alphabet

Two arguments, one named 'name' and the other one 'names'. 'data' is "the data to build the alphabet". And the explanation of what 'names' is (i.e. : the letters) makes one think that the 'data to build the alphabet is NOT the letters. What the hell does it all mean ?

Plus none of the examples of Alphabet? actually use Alphabet. Because that is called build_alphabet.

sage: build_alphabet(5,['a','b', 'c '])
...
ValueError: not possible

Plus it would really help to have an explanation of what 'hidden' means in the definition of hidden_keys and hidden_functIon (the 'i' is missing).

Nathann

comment:14 follow-up: Changed 11 months ago by tscrim

Nathann, you write the documentation then (as a review commit).

There is no class Alphabet; it just is an alias (or you can think of it as a facade) for build_alphabet. Also IMO, the examples are worth a 1000 words trying to explain all possible combinations in the docstring for the variables. From Webster: "Hidden (adjective): not seen or known, being out of sight or not readily apparent". So they are keys and a function on the keys which are not seen or are out of sight.

Furthermore, they are a few harmless merges of the develop branch in the history, but I don't want to wait an hour+ to switch between branches and test something in Sage. In fact, on most projects I've seen, the git history consists of many very small commits and can be called "messy".

comment:15 in reply to: ↑ 14 ; follow-up: Changed 11 months ago by ncohen

Hello !

Nathann, you write the documentation then (as a review commit).

I have absolutely no clue of what this function does. That's the problem. Show the documentation above to anybody you meet, and ask him to deduce what the function returns. If he gets it I am wrong, otherwise it needs to be rewritten.

There is no class Alphabet; it just is an alias (or you can think of it as a facade) for build_alphabet. Also IMO, the examples are worth a 1000 words trying to explain all possible combinations in the docstring for the variables.

Come on. It must be possible to write properly what the arguments actually mean O_o

From Webster: "Hidden (adjective): not seen or known, being out of sight or not readily apparent". So they are keys and a function on the keys which are not seen or are out of sight.

Thank you very much. Knowing what "hidden" means actually help.

Hey Travis, what do you think I am doing here ? Why would I come add comments to this ticket just to make it more painful for you ?
I honestly do not understand what "hidden" means in this context. No way. Does it mean that the letters are ignored ? That they just aren't displayed but that they are still stored somewhere ? Does this parameter have any actual use or should it be removed ?

That's what I want to find out. I have no idea what it does at the moment.

Furthermore, they are a few harmless merges of the develop branch in the history, but I don't want to wait an hour+ to switch between branches and test something in Sage.

There are 4 commits with unclear commit messages, and these four commits are as useful as none, as in the branch I uploaded. And I never had to recompile anything, I do not understand why you bring this up.

In fact, on most projects I've seen, the git history consists of many very small commits and can be called "messy".

How do they review stuff if each commit is meaningless ? O_o

Nathann

comment:16 in reply to: ↑ 15 ; follow-up: Changed 11 months ago by tscrim

Replying to ncohen:

I have absolutely no clue of what this function does. That's the problem. Show the documentation above to anybody you meet, and ask him to deduce what the function returns. If he gets it I am wrong, otherwise it needs to be rewritten.

If the documentation isn't clear, I read the code. IMO this function is not very complicated to understand from the code itself.

Come on. It must be possible to write properly what the arguments actually mean O_o

I wrote what they mean (at least, to me), but not how they are used (together).

Thank you very much. Knowing what "hidden" means actually help.

Hey Travis, what do you think I am doing here ? Why would I come add comments to this ticket just to make it more painful for you ?
I honestly do not understand what "hidden" means in this context. No way. Does it mean that the letters are ignored ? That they just aren't displayed but that they are still stored somewhere ? Does this parameter have any actual use or should it be removed ?

I'm sorry; I didn't realize that this was a language/definition issue (please don't post a reply in French, I think I know more Mandarin than French :p). I know you are trying to make Sage's doc more user friendly. I still think that the example which demonstrates these arguments will do much better than a long explanation in the INPUT: block. As I recall, the hidden keys/functions are used in the root systems.

That's what I want to find out. I have no idea what it does at the moment.

There are 4 commits with unclear commit messages, and these four commits are as useful as none, as in the branch I uploaded. And I never had to recompile anything, I do not understand why you bring this up.

Switch between branches which are not up to the latest development version (something like 6.1.beta1), and almost all of the cython files will be rebuilt because some low-level .pxi or .pxd file is changed.

How do they review stuff if each commit is meaningless ? O_o

The sum of the parts. The history is only there to see what changed where and when and by whom.

comment:17 in reply to: ↑ 16 Changed 11 months ago by ncohen

Yo Travis !

Okay, I am set to give up at least 3 hours of my life to make this doc clean. I am working on it right now and I have a question : why do you want to allow two arguments to be swapped in this case ?

    if isinstance(names, (int,long,Integer)) or names == Infinity: # Swap arguments...
        data,names = names,data 

Nathann

comment:18 Changed 11 months ago by ncohen

I also made a comment earlier about renaming "name" to "prefix". Do you think this would be a better name for "name" ?

Nathann

comment:19 Changed 11 months ago by ncohen

Also : what about changing all the build_alphabet to Alphabet in the docstring ? Do you think that BOTH names should be exported in the global namespace, and if so why ?

I think that "Alphabet" is more natural in Sage.

Nathann

comment:20 Changed 11 months ago by ncohen

  • Status changed from needs_work to needs_info

Okay. Here is where I am at. I am now waiting for your answers to my three questions above, and perhaps this patch can go and make those functions easier to use.

By the way : I never checkout a branch that is based on an earlier version of Sage than the last develop version. I do

git checkout d # my develop branch
git fetch trac public/whatever:whatever
git checkout -b rebased_whatever
git merge whatever

This way, no checkout and no compilation. And I can push an updated branch afterwards.

Nathann

comment:21 Changed 11 months ago by git

  • Commit changed from 03b3045881fbff482768e4d3efee772c83d58da8 to b39e393b108ff2a94bcd57dd341b26a7bfe36653

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

b39e393trac #15567: Doc fixes and comments in many places to understand something.

comment:22 Changed 11 months ago by ncohen

And it did take three hours ....

Last edited 11 months ago by ncohen (previous) (diff)

comment:23 Changed 11 months ago by ncohen

By the way, all my modifications in build_alphabet were meant to reorder the code only and shouldn't have any effect on the function's behaviour.

Nathann

comment:24 Changed 11 months ago by git

  • Commit changed from b39e393b108ff2a94bcd57dd341b26a7bfe36653 to 9310e0449bdba5a2424942bc0beb962e6f873c15

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

051153cFixes to alphabet and now using name arg in Family.
ad53e23Expanded documentation for #15567.
03b3045More tweaks to alphabet construction for #15567.
9310e04trac #15567: Doc fixes and comments in many places to understand something.

comment:25 Changed 11 months ago by ncohen

(just updated the last commit, a couple of typo)

comment:26 Changed 11 months ago by git

  • Commit changed from 9310e0449bdba5a2424942bc0beb962e6f873c15 to baaabd695e6aa168e02ef6ac2d933e9338e663b9

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

b31c406Fixed errors for #15567.
baaabd6Changed a ValueError to a TypeError. Documentation Tweak. #15567

comment:27 follow-up: Changed 11 months ago by tscrim

Your way of doing things only suppresses the commit message that's put on trac, there is still the merge in the git history. However I'm not for rewriting history on a branch on a ticket.

I've fixed the errors I found, standardized the exception messages, and did some other cleanup of documentation in family.py.

I swap the arguments to be consistent with PolynomialRing and because it makes it more user-friendly (at least less requirement of the user to look at the doc).

As I believe you are aware, name is not used as a prefix, so we let it be (and if you meant names, then see Vincent's request).

I don't exactly think Alphabet should be in the global namespace since it's not a class, but it doesn't bother me leaving it around since it is a natural (historical) guess/entry point (and if you want to deprecate it, please do so on another ticket).

Best,
Travis

comment:28 in reply to: ↑ 27 ; follow-up: Changed 11 months ago by ncohen

  • Reviewers set to Nathann Cohen

Your way of doing things only suppresses the commit message that's put on trac, there is still the merge in the git history. However I'm not for rewriting history on a branch on a ticket.

I do not understand. Why would it suppress the commit message put on trac ? Trac is perfectly able to display two commits at once ?.. O_o
And there is absolutely no history rewriting going on. I am just telling you how I avoid compiling commits based on old beta releases, which seemed to be a problem for you.

I've fixed the errors I found, standardized the exception messages, and did some other cleanup of documentation in family.py.

I see.

I swap the arguments to be consistent with PolynomialRing and because it makes it more user-friendly (at least less requirement of the user to look at the doc).

I think that's a terrible behaviour.

As I believe you are aware, name is not used as a prefix, so we let it be (and if you meant names, then see Vincent's request).

I meant 'names', of course.

Well.

At least now there is some doc in this function.

I don't exactly think Alphabet should be in the global namespace since it's not a class, but it doesn't bother me leaving it around since it is a natural (historical) guess/entry point (and if you want to deprecate it, please do so on another ticket).

I actually thought of deprecating "build_alphabet". Looks like a bad think to put in the global namespace.

Okay, positive review to that, and those methods are a bit better now than they were when we began this patch :-P

Nathann

P.S. : I will add a commit in a second to fix 2 typos

comment:29 Changed 11 months ago by git

  • Commit changed from baaabd695e6aa168e02ef6ac2d933e9338e663b9 to f73d436e2590d5a4134b813460464dd62267c6fe

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

f73d436trac #15567: two typos

comment:30 Changed 11 months ago by ncohen

  • Status changed from needs_info to positive_review

comment:31 in reply to: ↑ 28 ; follow-up: Changed 11 months ago by tscrim

Replying to ncohen:

Your way of doing things only suppresses the commit message that's put on trac, there is still the merge in the git history. However I'm not for rewriting history on a branch on a ticket.

I do not understand. Why would it suppress the commit message put on trac ? Trac is perfectly able to display two commits at once ?.. O_o
And there is absolutely no history rewriting going on. I am just telling you how I avoid compiling commits based on old beta releases, which seemed to be a problem for you.

You've called "git merge", which creates a merge commit. Or are you running a git rebase somewhere? My problem comes from branches that I have before the next beta version comes along, are you suggesting that I delete these branches, then re-pull all of them using of the above?

However 9310e04 vs. b39e393 is changing history.

Thanks for fixing those typos!

comment:32 in reply to: ↑ 31 Changed 11 months ago by ncohen

You've called "git merge", which creates a merge commit. Or are you running a git rebase somewhere? My problem comes from branches that I have before the next beta version comes along, are you suggesting that I delete these branches, then re-pull all of them using of the above?

Nonono ! But you can merge the branch into develop, instead of merging develop into your old branch. I create a branch on the latest develop, and call git merge the_old_branch. My new commit is above the latest beta, and as there was no checkout none of the files changed. You can then do a sage -b and only the files touched by the old branch changed.

However 9310e04 vs. b39e393 is changing history.

Ahahah. Well. That was my own last commit only (the hash of the other ones never changed, that part of the history was not rewritten), and that was only seconds after having pushed it. I think we can tolerate things like that :-P

Nathann

comment:33 follow-up: Changed 11 months ago by tscrim

Thanks again for reviewing this Nathann.

comment:34 in reply to: ↑ 33 Changed 11 months ago by ncohen

comment:35 Changed 11 months ago by tscrim

lolz

comment:36 Changed 11 months ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:37 Changed 11 months ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.