sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 480 forks source link

Posets: Adding function from categories to index of functions #18534

Closed f29946bc-ee7b-48cd-9abc-3445948c551d closed 9 years ago

f29946bc-ee7b-48cd-9abc-3445948c551d commented 9 years ago

It is more logical for a user to think "What can I do with this poset?" than "What functions are defined in this file?". This is a suggestion to add is_selfdual and is_lattice to index of functions in posets.py.

CC: @fchapoton @nathanncohen @VivianePons

Component: documentation

Author: Jori Mäntysalo

Branch/Commit: u/jmantysalo/posets__adding_function_from_categories_to_index_of_functions @ b31c216

Issue created by migration from https://trac.sagemath.org/ticket/18534

f29946bc-ee7b-48cd-9abc-3445948c551d commented 9 years ago

Branch: u/jmantysalo/posets__adding_function_from_categories_to_index_of_functions

f29946bc-ee7b-48cd-9abc-3445948c551d commented 9 years ago
comment:2

What you think of this?


New commits:

b31c216Added two functions to index of functions.
f29946bc-ee7b-48cd-9abc-3445948c551d commented 9 years ago

Commit: b31c216

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:3

I believe that you should move those functions from categories files to posets files. What you say in the ticket's description is already partly wrong: the index of poset function indicates the functions defined in FinitePoset, but not the functions inherited from more abstract classes: I expect that you do not plan to see all poset functions if we ever have an index of functions in lattices.py?

Nathann

f29946bc-ee7b-48cd-9abc-3445948c551d commented 9 years ago
comment:4

Replying to @nathanncohen:

I believe that you should move those functions from categories files to posets files. What you say in the ticket's description is already partly wrong: the index of poset function indicates the functions defined in FinitePoset, but not the functions inherited from more abstract classes: I expect that you do not plan to see all poset functions if we ever have an index of functions in lattices.py?

Sidenote, we do have index in lattices.py now: #18489.

I asked about moving functions about a year ago. There was some reasons for not doing it.

I think that everyone using lattices will know that a lattice is a poset, and so can find the functions. This is totally different in this case: plain mathematical knowledge can not reveal at all that some functions are available but from another file.

(There is a real corner case: Student playing with groups may not know what is magma or semigroup. In principle there should be no .is_abelian() (or something like that) directly on groups. It should be inherited from magmas. But that might be a problem from documentation point of view.)

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:5

We both agree that the index of Lattice should not list all Poset methods, and also that those two functions should appear in the index of Poset.

What I do not like is to have an index listing things which are not in its module, and that's why I believe that all poset functions should be in the same place, not scattered among posets.py and categories. I do not mind if somebody comes and gives this ticket a positive review, but as I do not quite agree with what is being done here, please give me the freedom to not stamp it myself.

Nathann

f29946bc-ee7b-48cd-9abc-3445948c551d commented 9 years ago
comment:7

Ping. Anybody wants to review a two line patch?

1adc0eef-8957-46d9-975b-2dd71dfbd9ba commented 9 years ago
comment:8

I finally have a chance to start reviewing things again.

Is there any chance you could look up the justification for keeping some poset functions in posets.py and some in categories? Because I agree that unifying them would be the better solution.

Also, why does the index of functions not make any reference to other modules that it may be inheriting functions from? Even if we can expect that (for example) a user will know that every lattice is a poset, the documentation should directly provide a link from the index of functions for Lattices to the index of functions for Posets, rather than forcing them to manually find the relevant page.

f29946bc-ee7b-48cd-9abc-3445948c551d commented 9 years ago
comment:9

Replying to @kevindilks:

Is there any chance you could look up the justification for keeping some poset functions in posets.py and some in categories? Because I agree that unifying them would be the better solution.

This has been discussed on sage-devel. I did not understood all of it, but in any case there will be functions in two places.

Also, why does the index of functions not make any reference to other modules that it may be inheriting functions from? Even if we can expect that (for example) a user will know that every lattice is a poset, the documentation should directly provide a link from the index of functions for Lattices to the index of functions for Posets, rather than forcing them to manually find the relevant page.

Can be done. However, there is BIG difference here: it needs "only" mathematical knowledge to know that a lattice is a poset, but it needs knowledge of Sage internals to know that more functions may be found on categories.

But this is another story and does not belong to this patch.

f29946bc-ee7b-48cd-9abc-3445948c551d commented 9 years ago
comment:10

Snif. My poset documentation polishing project (see #18959, #18925, #18941) will stuck, as nobody wants to review this, but either has said that this should be rejected.

Two lines only...

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:11

Well. You say that nobody rejected it, but I am personally against the changes from two of those tickets. You are only right in the sense that I don't believe that 'not agreeing' is sufficient to 'reject it'.

By the way, you should really stop with those two-lines tickets and merge those superficial changes into something else (if you find somebody who agrees with it).

f29946bc-ee7b-48cd-9abc-3445948c551d commented 9 years ago
comment:12

Replying to @nathanncohen:

Well. You say that nobody rejected it, but I am personally against the changes from two of those tickets. You are only right in the sense that I don't believe that 'not agreeing' is sufficient to 'reject it'.

By the way, you should really stop with those two-lines tickets and merge those superficial changes into something else (if you find somebody who agrees with it).

Already lowering ticket counter with #19023.

I think that the point of this ticket is not about those two lines. It is the discussion about how to enhance documentation of the software.

I would say "reject" to leaving things as they are. This patch is my suggestion one. Suggestion two is to add index to category of posets, add index to category of finite posets, and link them at the beginning of index of finite posets. Suggestion three is to make a new page that contains index of poset functions and a short introduction to posets in Sage. All these suggestions assumes that the code itself will not be refactored.

(You may mean that I should stop doing tickets like #18925. If that is the case, I must say that it is hard. To get unified interface it is needed to sometimes go throught a bigger part of Sage. OTOH if I make a big but simple patch of doctrings, it will take a year to write, will needs rebasing ten times, and may never get positive_review.)

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:13

I would say "reject" to leaving things as they are. This patch is my suggestion one. Suggestion two is to add index to category of posets, add index to category of finite posets, and link them at the beginning of index of finite posets. Suggestion three is to make a new page that contains index of poset functions and a short introduction to posets in Sage. All these suggestions assumes that the code itself will not be refactored.

Move the two functions from categories to posets.py and the problem is solved. I don't see the added value of having those two lone functions in another file, where nobody will find them. Move them to posets.py and they can obviously be added to the doc.

(You may mean that I should stop doing tickets like #18925. If that is the case, I must say that it is hard. To get unified interface it is needed to sometimes go throught a bigger part of Sage. OTOH if I make a big but simple patch of doctrings, it will take a year to write, will needs rebasing ten times, and may never get positive_review.)

We could start getting rid of those big index, and of the duplication of [1) the description of the function that follows the name of the function in the index 2) the first line of the docstring of a function] by using #18926.

Once this is done, the individual modifications do not need to be rebased as often, for the parts of code that get modified are not all located in the index: only the individual docstrings need be modified, and git has less problems with that.

Nathann

f29946bc-ee7b-48cd-9abc-3445948c551d commented 9 years ago
comment:14

Replying to @nathanncohen:

Move the two functions from categories to posets.py and the problem is solved. I don't see the added value of having those two lone functions in another file, where nobody will find them. Move them to posets.py and they can obviously be added to the doc.

That would be my suggestion number zero. But I wrote about that on sage-devel, and was clearly rejected. (And this is not only about two function, see for example is_antichain_of_poset(). -- it does not help if you look only the category of finite posets.)

We could start getting rid of those big index, and of the duplication of [1) the description of the function that follows the name of the function in the index 2) the first line of the docstring of a function] by using #18926.

The fact that #18926 is a great improvement (thanks!) does not mean that it will make documentation good. Good index is arranged by topic. And actually I have used a little different phrasing in the index compared to function.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:15

That would be my suggestion number zero. But I wrote about that on sage-devel, and was clearly rejected.

Is "Clearly rejected" the difference between my "I don't like it, so I will not review it" and somebody else's "I don't like it, so you must not do it"?

I still don't know what there is to reject. There was a Poset class with everything inside, then somebody created a Poset category and suddenly it magically becomes "the best place" to store code, even though there is nothing in there.

The fact that #18926 is a great improvement (thanks!) does not mean that it will make documentation good. Good index is arranged by topic. And actually I have used a little different phrasing in the index compared to function.

You can do it with #18926 too. Instead of telling it to print all the functions of the class, you can create several small tables from lists of functions. This way the list is always sorted lexicographically, and you also don't have to copy/paste the first line of each docstring (which gets updated automatically).

Furthermore, because we will have a Python list of the function that are indexed, we can later "check" that all functions appear, which we cannot do at the moment.

Nathann

f29946bc-ee7b-48cd-9abc-3445948c551d commented 9 years ago
comment:16

Replying to @nathanncohen:

Is "Clearly rejected" the difference between my "I don't like it, so I will not review it" and somebody else's "I don't like it, so you must not do it"?

I remember that it was several peoples "I don't like". And now when I check, "several" was actually "two"... They were Travis and Nicolas, and I am not sure about Nicolas. (But wait - was this discussed twice? Not sure.)

I still don't know what there is to reject. There was a Poset class with everything inside, then somebody created a Poset category and suddenly it magically becomes "the best place" to store code, even though there is nothing in there.

I don't know. There was some explanation, of which I think I didn't understood all.

The fact that #18926 is a great improvement (thanks!) does not mean that it will make documentation good. Good index is arranged by topic. And actually I have used a little different phrasing in the index compared to function.

You can do it with #18926 too. Instead of telling it to print all the functions of the class, you can create several small tables from lists of functions.

That sounds better. Lexicographic order is not always the best possible, but that's a minor thing.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:17

I don't know. There was some explanation, of which I think I didn't understood all.

I see. Well, if you ever want to move two functions back to the Poset class, just add me in Cc.

That sounds better. Lexicographic order is not always the best possible, but that's a minor thing.

You can disable it, too.

If it makes sense, we could even update the code so that it can take as argument a list of functions "with label", group the functions according to the lablels and build the corresponding tables. Should take 10 lines at most.

Nathann

f29946bc-ee7b-48cd-9abc-3445948c551d commented 9 years ago
comment:18

It seems that majority on sage-devel are against this. See for example thread https://groups.google.com/forum/#!topic/sage-devel/rnye9oBdgKk . Hence I move this to wontfix-milestone.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:19

wontfix tickets must be set to positive_review, otherwise they will not be closed.