-
-
Notifications
You must be signed in to change notification settings - Fork 660
Fixed the doc sets/recursively_enumerated_set.py
#37331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed the doc sets/recursively_enumerated_set.py
#37331
Conversation
I'm not sure creating a new media is a good idea, since it's hard to change. Instead, try following how combinat/bijectionist does it. You can see the result here |
Hi,
But, it was kind of difficult to manage the alignments, font size, inverted commas etc in the pyx file itself. I will try to implement it and commit. Thank you. |
Cool! Could you try to remove the bullet points alltogether? Also, the arrow labelled 2) must point to an arrow, not to a vertex of the tree. In fact, either the two arrows labelled 1) and 2) (which "explain" the diagram) should look very different from the other arrows, or should be removed entirely (together with the text "an initial element" and "a function ..."). I would actually vote for removing them: the concept is not all that difficult to understand to warrant such a complicated figure. |
Great job! Thank you for looking so carefully through the whole text! Did you use a tool or did you do it by hand? |
Thank you for your comments :-) The initial part was done by me; later I used Grammarly to cross-check things. It suggested some modifications like replacing 'depth first search' with 'depth-first search', 'non negative' with 'non-negative' and so on, which I did not appreciate by going through some other documentation of Sage. Also, I wanted to say that maybe some Sphinx directive like |
It would be cool if you could finish this, so it makes it into the upcoming release! |
Thank you! One question: it might be even better to simplify to single quotes, as one might be used to from working with sage. I.e.,
(and without the \hspace) What do you think? |
Consider the following:
Some observations:
|
I'd go with
It is not all that nice, but it's readable and very simple. |
Cool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! I would like to wait for the linter, to make sure that there is no trailing whitespace. Unfortunately, I do not have the rights to approve the workflow, it seems!
sagemathgh-37331: Fixed the doc `sets/recursively_enumerated_set.py` <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> Modified the doc in `sets/recursively_enumerated_set.py ` (in the section `Example: Forest structure`) to display the HTML nicely; also corrected some typos. <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> Sphinx `.. MATH::` directive is unable to render the `picture` environment in LaTeX, as a result of which the image is not begin displayed nicely in [sets/recursively_enumerated_set](https://doc.sagema th.org/html/en/reference/sets/sage/sets/recursively_enumerated_set.html) . I generated the pdf and png of the required image by compiling the LaTeX code and attached the image. The inspiration is from `Figure: The five complete binary trees with four leaves` in [combinat/tutorial](http s://doc.sagemath.org/html/en/reference/combinat/sage/combinat/tutorial.h tml). <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> This PR resolves an open issue, that is -- sagemath#37084. <!-- If your change requires a documentation PR, please link it appropriately. --> The link of the original documentation -- [sets/recursively_enumerated_s et](https://doc.sagemath.org/html/en/reference/sets/sage/sets/recursivel y_enumerated_set.html). ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. (Not required it seems.) - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> None. <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37331 Reported by: Janmenjaya Panda Reviewer(s): Martin Rubey
EXAMPLES: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be removed. What follows are examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I don't think it is all that strange, but you could considering it as wrong type of heading is used for the sub portions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, when I saw it I thought that for some reason (eg., bad markup) the examples were not typeset.
Would it be OK for you if we add instead EXAMPLES::
right before the examples in each subsection?
I thought that the EXAMPLES::
markup is only used in docstrings of classes, methods and functions, not at module level, and I could not find file where we have examples at module level interoduced with EXAMPLES::
.
But in any case, I agree that the sizing of the headings is not good. Do you have a best-practice example with a similar structure (i.e., with a guided tour at module level)? I only know of bijectionist.py
, findstat.py
, growth.py
, but they have all been written by myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole list are the examples broken down into different sections. We do have other examples scattered around, but they are less frequent as the class-level doc is usually a better place for ?
access (e.g., libgap_group.py
has some module-level doc examples, but likely not the best place). So I would prefer to keep it unchanged.
We don't really have a best practice, but perhaps .. RUBRIC::
? H4 level might be fine too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops,
I just reverted. 😓
Apologies for the confusion. Please let me know what is to be done here.
Actually, in the original doc, the EXAMPLES::
tag exists just before each of the examples in almost all of the places, except for the initial few paragraphs/ sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I don't like it at all, but I don't care enough.
For me, "EXAMPLES:" looks like a heading. I would say that, ordinarily, a heading can only be followed by a sub-heading - however, "EXAMPLES:" is always just in capital letters, which would be the smallest possible heading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion,
The initial sections including No hypothesis on the structure
, Symmetric structure
, Graded structure
and Forest structure
-- each of these has a brief description regarding the structure followed by examples (just before which I added the EXAMPLES::
tag). In the Forest structure section, there are three consecutive examples, I removed the large section heading of Example: Forest structure
and Example: Forest structure 2
and added a single tag EXAMPLES::
before the start of the examples just after the forest structure.
The inclusion of the EXAMPLES::
tag just before the example makes more sense than adding a single tag at the beginning of the entire document just after the name of the author and just before the No hypothesis on the structure
.
I hope this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, The initial sections including
No hypothesis on the structure
,Symmetric structure
,Graded structure
andForest structure
-- each of these has a brief description regarding the structure followed by examples (just before which I added theEXAMPLES::
tag).
That's not generally true. I only see this for the "no hypothesis" section.
In the Forest structure section, there are three consecutive examples, I removed the large section heading of
Example: Forest structure
andExample: Forest structure 2
and added a single tagEXAMPLES::
before the start of the examples just after the forest structure.The inclusion of the
EXAMPLES::
tag just before the example makes more sense than adding a single tag at the beginning of the entire document just after the name of the author and just before theNo hypothesis on the structure
.
No, it doesn't. All of those that follow in the original EXAMPLES:
are examples of the module that are explained in detail. They don't talk about the assumptions/structure needed to be satisfies (e.g., a formal definition) or have any generality.
As further evidence, your added EXAMPLES::
is following after each block already described the example. This is clearly out of place (plus the English sentence syntax is wrong with the previous lines ending with the colon).
Either this needs to be reverted or the doc needs to be completely rewritten in a different format. We can address @mantepse's issue by replacing the header part with a smaller block; e.g,:
-Example: Forest structure
--------------------------
+.. RUBRIC:: Forest structure [Ex num]
That should make it look much better in the compiled doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool.
I have understood your point and I have made the changes accordingly (as of now).
Actually, I did not look at the initial sections of the doc from this point of view (that -- the initial sections are the examples for some instances of the recursively enumerated set under some assumptions) and yes, they lack the proper formal definition. We should discuss about it.
Also, I am thankful to everyone for this discussion :-)
@@ -406,7 +386,7 @@ def RecursivelyEnumeratedSet(seeds, successors, structure=None, | |||
|
|||
.. WARNING:: | |||
|
|||
If you do not set the good structure, you might obtain bad results, | |||
If you do not set a valid structure, you might obtain bad results, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do not set a valid structure, you might obtain bad results, | |
If you do not set a good structure, you might obtain bad results, |
Valid is not quite strong enough here I think. In either case, it is better to be a bit more vague.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I am surprised to learn that "good" is not frenglish!
More importantly, I don't understand the sentence, it seems contradictory to me:
- valid is not strong enough
- it is better to be more vague
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term "good" is not well-defined (more vague), but it implies that there could be something more needed than what is specified as valid (stronger conditions are needed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the following sound good?
Instead of the statement:
If you do not set a valid/ good structure, you might obtain bad results, like elements generated twice:
we may write:
A poorly defined recursion might lead to an unexpected result, such as some of the elements being generated multiple times:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that isn't quite right and becomes too vague as it doesn't reference the structure of the poset/recursion. It would be best just to leave it as "good" with changing "the" to "a".
90cdce0
to
de98279
Compare
It seems that there have been too many commits. |
No, I don't think so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think so either. It has useful information of things that have been tried.
@tscrim, did you look at the generated html? I think it looks terrible :-( I am unhappy about the |
Hello, I want to make two points.
This is a temporary solution. I believe we should include the definitions of
This seems like a more robust and better solution concerning the examples than the existing one. This will address both the issues raised by @mantepse and @tscrim and give a better overview of the doc.
Please consider putting |
Also, the Lint test is failing. I suppose, once again there is some trailing whitespace. |
@mantepse I don’t think it looks that bad, but okay. Let me again suggest keeping how the original version was but changing all of the headings to be I don’t think it is useful to duplicate documentation with the (precise) definitions. They are examples. If someone wants the definitions, they can look later in the doc. I agree with @mantepse that the quotes in the latex looks pretty bad in the html. I would remove them all and replace the empty with The lint test is very reliable. If it is failing, then it needs to be fixed. |
@mantepse What do you think about the doc now? |
Looks OK! Possibly the module heading should be |
Is it required to change the file name from |
NO! |
Documentation preview for this PR (built with commit 162c152; changes) is ready! 🎉 |
@mantepse Any additional comments? I am ready to set this to a positive review. |
Perfect! |
Modified the doc in
sets/recursively_enumerated_set.py
(in the sectionExample: Forest structure
) to display the HTML nicely; also corrected some typos.Sphinx
.. MATH::
directive is unable to render thepicture
environment in LaTeX, as a result of which the image is not begin displayed nicely in sets/recursively_enumerated_set. I generated the pdf and png of the required image by compiling the LaTeX code and attached the image. The inspiration is fromFigure: The five complete binary trees with four leaves
in combinat/tutorial.Fixes #37084.
The link of the original documentation -- sets/recursively_enumerated_set.
📝 Checklist
⌛ Dependencies
None.