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

Improved matrix documentation #1982

Merged
merged 2 commits into from
Feb 7, 2025
Merged

Conversation

emikelsons
Copy link
Contributor

Improved matrix documentation by splitting matrix.md into 3 disjoint files
Any suggestions on possible changes are welcome
This is addressing #1981

@emikelsons emikelsons force-pushed the DocImprovements branch 4 times, most recently from 818e781 to 61d0040 Compare February 5, 2025 15:33
@HereAround
Copy link
Contributor

@fingolfin @emikelsons and I just finished working out a first draft. Please take a look. more than happy to discuss more.

Copy link
Member

@thofma thofma left a comment

Choose a reason for hiding this comment

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

Like the direction. Left a few comments.


It is also possible to create matrices (in a matrix space only) directly, without first
It is possible to create matrices directly, without first
Copy link
Member

Choose a reason for hiding this comment

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

Since matrices spaces are not mentioned anymore, this sentence needs to be adjusted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention was to point out that matrix spaces aren't always necessary. However, per your suggestion I have adjusted the text slightly

[t + 1 t 1]
[ t^2 t t]
[ -2 t + 2 t^2 + t + 1]
julia> A = S([K(0) 2a + 3 a^2 + 1; a^2 - 2 a - 1 2a; a^2 + 3a + 1 2a K(1)])
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make this matrix space free?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have now completely separated out all matrix space examples and have added generic matrix examples, where necessary

Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.33%. Comparing base (070ea63) to head (cc64c43).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1982      +/-   ##
==========================================
- Coverage   88.34%   88.33%   -0.01%     
==========================================
  Files         124      125       +1     
  Lines       31404    31416      +12     
==========================================
+ Hits        27743    27752       +9     
- Misses       3661     3664       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ 2 3 1]
[ t t + 1 t + 2]
[-1 t^2 t^3]
```@example
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
```@example
```jldoctest

please use jldoctest instead of @example wherever possible. Examples will just bitrot, we won't notice that they are out of date and need an update. See e.g. Nemocas/Nemo.jl#1683

Copy link
Contributor Author

@emikelsons emikelsons Feb 6, 2025

Choose a reason for hiding this comment

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

I initially added them as jldoctests, but I was getting a doctest error, so I changed it to @example. I believe the error was due to the fact, that I call @docs for the same function in 2 different .md files, but the jldoctests were different

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, that sound like a different problem I can help you with that maybe makes this issue go away.

You can only include each docstring once by default. If you want to include it multiple times, you need to add canoncial=false to all but one occurrence. See e.g. https://github.com/oscar-system/Oscar.jl/blob/ca6338cf6de3345876d1db317a3230dce15b7ab7/docs/src/LieTheory/root_systems.md?plain=1#L50
The one place without canonical=false is the one that is linked to if you have a [`weyl_group(::RootSystem)`](@ref) somewhere.

Could you try this and then revert the example blocks back to doctests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The doctests here are completely independent from all of this, as they are not part of the docstrings but just happen to be in the markdown file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have now reverted back to jldoctests with the inclusion of 'canonical=false'. Hopefully, the tests should pass now

Copy link
Collaborator

Choose a reason for hiding this comment

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

CI is green now 🎉

@thofma thofma merged commit 04b9871 into Nemocas:master Feb 7, 2025
28 of 29 checks passed
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.

4 participants