Skip to content
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

Documentation: vertices_and_rays and maximal_polyhedra (tropical variety) #3918

Merged

Conversation

MarieKaltoft
Copy link
Contributor

@MarieKaltoft MarieKaltoft commented Jul 5, 2024

See discussion.

I am working on adding some examples to the online documentation in the tropical geometry section. These examples aim to make the commands vertices_and_rays and maximal_polyhedra easier to understand, as well as the relation between them.

Following was added as a comment, when sent to review:
Comment for reviewer: I have now added an example of a tropical hypersurface. The example is rather rudimentary and quite spelled out, and I am of course willing to correct/change things, if it should not be as spelled out in the documentation. However, as a Master's student, who only recently got into tropical geometry, this is what I believe would have helped me, when I was learning to use these tools.

Copy link

codecov bot commented Jul 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.56%. Comparing base (953f8bf) to head (e8f3f27).
Report is 128 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3918      +/-   ##
==========================================
+ Coverage   83.96%   84.56%   +0.60%     
==========================================
  Files         591      612      +21     
  Lines       81380    83034    +1654     
==========================================
+ Hits        68329    70218    +1889     
+ Misses      13051    12816     -235     

see 124 files with indirect coverage changes

@fingolfin
Copy link
Member

Currently this just adds a line stating an intent to add documentation, but does nothing else. Is the plan to change that? Do you already know when roughly you'll have time to do it?

@MarieKaltoft
Copy link
Contributor Author

Currently this just adds a line stating an intent to add documentation, but does nothing else. Is the plan to change that? Do you already know when roughly you'll have time to do it?

@fingolfin Sorry that I just left that hanging! I have been busy with other commitments. My intention was to start looking more at it next week (and finish it in August). Is that alright?

Added example
Changed "Examples" to "Example"
Changed code-environments to jldoctest
@joschmitt
Copy link
Member

Can you put

    ```@meta
    CurrentModule = Oscar
    DocTestSetup = Oscar.doctestsetup()
    ```

at the top of this file? This is necessary to make the doc tests work. (I'm not sure whether this is written down somewhere and I hope I'm not saying anything wrong here.)
Also, since the different code blocks use variables from an earlier code block, you need to supply a label to the code blocks like explained here https://documenter.juliadocs.org/stable/man/doctests/#Preserving-Definitions-Between-Blocks .

Added meta setup for doctest and labels for codeblocks
@MarieKaltoft
Copy link
Contributor Author

I am unsure what it means that doctest (1.11-nightly, ubuntu-latest) fails? Can anyone help with this? Maybe @joschmitt?

@thofma
Copy link
Collaborator

thofma commented Aug 28, 2024

You can ignore it (it is reported as #4047).

@joschmitt
Copy link
Member

@MarieKaltoft once you are done here, please mark the pull request as "ready for review".

Moved the meta setup to top of document to match structure of other documents.
@MarieKaltoft
Copy link
Contributor Author

MarieKaltoft commented Aug 28, 2024

Comment for reviewer: I have now added an example of a tropical hypersurface. The example is rather rudimentary and quite spelled out, and I am of course willing to correct/change things, if it should not be as spelled out in the documentation. However, as a Master's student, who only recently got into tropical geometry, this is what I believe would have helped me, when I was learning to use these tools.

@MarieKaltoft MarieKaltoft marked this pull request as ready for review August 28, 2024 08:33
@joschmitt joschmitt requested a review from YueRen August 28, 2024 08:35
Copy link
Member

@YueRen YueRen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @MarieKaltoft

Thanks a lot for the wonderful addition to the documentation! Just a couple of nitpicky things (sorry!):

(a) For the sake of consistence with the rest of the tropical documentation, could you please rename TRing to T and THg to TropH?
(b) Before showing off vertices.(maxPolTg) and rays.(maxPolTg), could you please add a short code block showing off IncidenceMatrix(maxPolTg)? I personally find it incredibly useful and I recommend it to anybody who wants to know more about the maximal polyhedra of a polyhedral complex.

Changed TRing to T and THg to TropH for consistency.
Added IncidenceMatrix and rewrote text to fit.
@MarieKaltoft
Copy link
Contributor Author

Hi @YueRen

Thanks for the feedback! I have changed the example accordingly. It was actually IncidenceMatrix, which I found so confusing initially, so I have now tried to include it in a way that also explains, what it actually outputs. I hope it makes sense.

Sorry that I took so long to get back - I don't seem to be getting all the notifications from GitHub that I should...

Copy link
Member

@YueRen YueRen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, more than happy for this to be merged!

@joschmitt
Copy link
Member

Thank you @MarieKaltoft!

@joschmitt joschmitt merged commit 56eaf4a into oscar-system:master Sep 8, 2024
28 checks passed
HechtiDerLachs pushed a commit to HechtiDerLachs/Oscar.jl that referenced this pull request Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants