Opened 6 years ago

Closed 6 years ago

#18534 closed enhancement (wontfix)

Posets: Adding function from categories to index of functions

Reported by: jmantysalo Owned by:
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: documentation Keywords:
Cc: chapoton, ncohen, VivianePons Merged in:
Authors: Jori Mäntysalo Reviewers:
Report Upstream: N/A Work issues:
Branch: u/jmantysalo/posets__adding_function_from_categories_to_index_of_functions (Commits, GitHub, GitLab) Commit: b31c216bae73e63b9066778e2a3d569536ca91d2
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (20)

comment:1 Changed 6 years ago by jmantysalo

  • Branch set to u/jmantysalo/posets__adding_function_from_categories_to_index_of_functions

comment:2 Changed 6 years ago by jmantysalo

  • Cc chapoton ncohen added
  • Commit set to b31c216bae73e63b9066778e2a3d569536ca91d2
  • Status changed from new to needs_review

What you think of this?


New commits:

b31c216Added two functions to index of functions.

comment:3 follow-up: Changed 6 years ago by ncohen

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

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

Replying to ncohen:

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.)

comment:5 Changed 6 years ago by ncohen

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

comment:6 Changed 6 years ago by jmantysalo

  • Cc VivianePons added

comment:7 Changed 6 years ago by jmantysalo

Ping. Anybody wants to review a two line patch?

comment:8 follow-up: Changed 6 years ago by kdilks

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.

comment:9 in reply to: ↑ 8 Changed 6 years ago by jmantysalo

Replying to kdilks:

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.

comment:10 Changed 6 years ago by jmantysalo

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...

comment:11 follow-up: Changed 6 years ago by ncohen

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).

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

Replying to ncohen:

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.)

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

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

comment:14 in reply to: ↑ 13 ; follow-up: Changed 6 years ago by jmantysalo

Replying to ncohen:

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.

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

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

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

Replying to ncohen:

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.

comment:17 in reply to: ↑ 16 Changed 6 years ago by ncohen

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

comment:18 Changed 6 years ago by jmantysalo

  • Milestone changed from sage-6.8 to sage-duplicate/invalid/wontfix

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.

comment:19 Changed 6 years ago by ncohen

  • Status changed from needs_review to positive_review

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

comment:20 Changed 6 years ago by vbraun

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