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

show GAP help in Julia with 'GAP.show_GAP_help' and via ? #251

Merged
merged 7 commits into from
Jul 9, 2019

Conversation

ThomasBreuer
Copy link
Member

This is a first approximation.

The functions in the file helpstring.g arose from those
in GAP's library file helpbase.gi.
The idea is to extract the requested information
from the text format files of the GAP manuals,
and to collect it in a string,
also in the case where several matches are found.

Currently the following problems are known.

  • When the last subsection of a section is requested
    then also the part of the next section (in the same chapter)
    until the start of its first subsection is shown;
    apparently the sections themselves have no entry numbers
    in the sense of HELP_BOOK_HANDLER.(book.handler).HelpData.

    (An example is SizeScreen.)

  • Some pieces of GAP help are very long,
    an example is the entry for Size in the HAP package.

On the other hand, duplicate entries are filtered out,
which should perhaps also done in GAP's help system.

@codecov
Copy link

codecov bot commented Jun 13, 2019

Codecov Report

Merging #251 into master will decrease coverage by 1.03%.
The diff coverage is 55.36%.

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
- Coverage   70.21%   69.18%   -1.04%     
==========================================
  Files          51       54       +3     
  Lines        3280     3605     +325     
==========================================
+ Hits         2303     2494     +191     
- Misses        977     1111     +134
Impacted Files Coverage Δ
src/GAP.jl 91.89% <ø> (+2.41%) ⬆️
test/help.jl 100% <100%> (ø)
pkg/GAPJulia/JuliaInterface/read.g 100% <100%> (ø) ⬆️
src/help.jl 16.66% <16.66%> (ø)
pkg/GAPJulia/JuliaInterface/gap/helpstring.g 55.78% <55.78%> (ø)
src/julia_to_gap.jl 95.38% <0%> (+2.84%) ⬆️
... and 3 more

@sebasguts
Copy link
Contributor

sebasguts commented Jun 17, 2019

I updated this PR and added some functionality to display the GAP help when the Julia help mode is called on GAP.Globals.Foo

@sebasguts sebasguts changed the title show GAP help in Julia with 'GAP.show_GAP_help' show GAP help in Julia with 'GAP.show_GAP_help' and via ? Jun 17, 2019
@sebasguts
Copy link
Contributor

@ThomasBreuer @mohamed-barakat @fingolfin Please have a look.

I am going to rebase this PR once all the file-reordering is done.

@mohamed-barakat
Copy link
Contributor

julia> GAP.show_GAP_help("HomalgMatrix")

works.

In the notebook it looks like this:

https://gist.github.com/mohamed-barakat/f0335ef22e4a8618dd195cca0b6ac9b4

@ThomasBreuer
Copy link
Member Author

@mohamed-barakat Thanks for testing this feature.
I had not intended this way of displaying GAP's help for the notebook situation,
opening the HTML help (similar to what GAP's JupyterKernel package does)
would be a better solution.

@wbhart wbhart mentioned this pull request Jun 17, 2019
42 tasks
@sebasguts
Copy link
Contributor

I think even in the notebook it looks kinda reasonable.
With this PR, if you trigger the notebook help by pressing shift+tab either once, twice, or trice, it even gets displayed with colors. I would think this is reasonable.

@ThomasBreuer
Copy link
Member Author

@sebasguts The first lines of help output in the notebook are broken into lines in a strange way.
I will fix this.

@sebasguts
Copy link
Contributor

Well, just when you use that printing function. If you use shift + tab, it actually prints it reasonable in Jupyter (which is also the prefered way to display help in Jupyter notebooks)

@mohamed-barakat
Copy link
Contributor

@mohamed-barakat Thanks for testing this feature.
I had not intended this way of displaying GAP's help for the notebook situation,
opening the HTML help (similar to what GAP's JupyterKernel package does)
would be a better solution.

@ThomasBreuer: Actually I also like to have this "inline" option for presentations.

@mohamed-barakat
Copy link
Contributor

@ThomasBreuer @mohamed-barakat @fingolfin Please have a look.

I am going to rebase this PR once all the file-reordering is done.

@sebasguts: The updated link

https://gist.github.com/mohamed-barakat/f0335ef22e4a8618dd195cca0b6ac9b4

shows that also ?HomalgMatrix works.

@mohamed-barakat
Copy link
Contributor

Well, just when you use that printing function. If you use shift + tab, it actually prints it reasonable in Jupyter (which is also the prefered way to display help in Jupyter notebooks)

Also this works fine for me.

@mohamed-barakat
Copy link
Contributor

One last comment. I get colors in the Jupyter notebook which are not visible in the gist link.

@sebasguts
Copy link
Contributor

@mohamed-barakat which colors do you mean? The ones in the ouput of show_GAP_help?

@mohamed-barakat
Copy link
Contributor

@mohamed-barakat which colors do you mean? The ones in the ouput of show_GAP_help?

@sebasguts: yes, although the output of show_GAP_help in the gist-link has no colors.

@sebasguts
Copy link
Contributor

Indeed. But you can see them on nbviewer

@mohamed-barakat
Copy link
Contributor

Could you please resolve the conflicts in this PR? I would like to merge it.

@fingolfin
Copy link
Member

@mohamed-barakat we'll resolve them once PR #249 is merged, not before. Also, I'd like to review this PR once, too

@@ -0,0 +1,536 @@
#############################################################################
##
## copied from GAP's 'lib/helpbase.gi', then adjusted
Copy link
Member

Choose a reason for hiding this comment

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

OK, that's pretty close of course. If lib/helpbase.gi is ever changed, we'd have to track that, too. I wonder if in the future, we might work on refactoring the code on the GAP side so that we can use it directly, instead of replicating it here (e.g. the code Thomas wrote here could be put into GAP, and then lib/helpbase.gi rewritten to use it).

pkg/GAPJulia/JuliaInterface/julia/ccalls.jl Outdated Show resolved Hide resolved
pkg/GAPJulia/JuliaInterface/julia/gap.jl Outdated Show resolved Hide resolved
@mohamed-barakat
Copy link
Contributor

mohamed-barakat commented Jun 19, 2019

Indeed. But you can see them on nbviewer

@sebasguts: Excellent.

@sebasguts
Copy link
Contributor

@mohamed-barakat I am going to wait untill #249 is merged, because otherwise I would resolve conflicts twice.

@sebasguts
Copy link
Contributor

I will move the code over into a separate file. I would guess the comments about the GAP code are optional @fingolfin? Because I really do not want to wait with this until we have refactored the GAP help system.

@sebasguts
Copy link
Contributor

I addressed the comments and updated this PR

@sebasguts
Copy link
Contributor

I rebased the PR

@fingolfin
Copy link
Member

@sebasguts the GAP refactoring remarks as "not even optional": they were ramblings about the distant future. Don't worry about them here :-)

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

I've rebase this again to resolve a recent conflict, and removed pkg/GAPJulia/JuliaInterface/julia/libgap.jl (which was accidentally added back). Assuming tests pass now, I think this is OK to merge.

@fingolfin
Copy link
Member

OK, looking at coverage made me realize that we don't have any tests for this. Could we please add some? Possible both on the GAP and the Julia side?

@ThomasBreuer
Copy link
Member Author

@fingolfin The only tests I can think of are tests whether calling the functions in question return some strings, regardless of the contents. If this is sufficient then I can add such tests.

@ThomasBreuer
Copy link
Member Author

I think that the reported code coverage is wrong.

For example, the function HELP_DESC_MATCH is called in the tests,
but if I understand the output correctly then https://codecov.io/gh/oscar-system/GAP.jl/compare/adcfc6dd37646408e31e7c88a8583113738b0036...b7d69210cee9dac1d99bc66af609236d40ef5e7b/diff claims that the lines of the function are counted as missed.

(I have already tried to rebase the pull request, what else should I do?)

@fingolfin
Copy link
Member

@ThomasBreuer the coverage data looks correct to me. I don't see a direct call to HELP_DESC_MATCH in the tests either. I assume you have verified locally that it actually gets called by the tests (e.g. by inserting an Error or Print into it). My guess would be that the problem is that we do not compile the GAP manual on Travis?

@ThomasBreuer
Copy link
Member Author

@fingolfin thanks for your hint.
Yes, the problem is that the .txt and manual.six files of the manuals are not available.

In this situation, requests for help just reply that nothing reasonable was found,
thus I do not see a way to provide sensible tests for this pull request.

(The current code can run into errors when the manuals are missing/incomplete,
for example if a manual.six file is available but a .txt file is missing.
I am going to update the pull request.)

@fingolfin
Copy link
Member

Well of course we could compiled the GAP manual as well. Probably a good idea anyway, as users will want to have access to it. Well, or we switch to using GAP 4.10.2, which of course includes prebuilt manuals.

@fingolfin
Copy link
Member

@ThomasBreuer on master, we now build the manuals of JuliaInterface and JuliaExperimental. So if you rebase this, perhaps coverage will improve.

@fingolfin
Copy link
Member

As an experiment, I cherry-picked commit 6466e3d onto this PR, to see if it has any effect on the coverage.

ThomasBreuer and others added 5 commits July 3, 2019 22:42
This is a first approximation.

The functions in the file `helpstring.g` arose from those
in GAP's library file `helpbase.gi`.
The idea is to extract the requested information
from the text format files of the GAP manuals,
and to collect it in a string,
also in the case where several matches are found.

Currently the following problems are known.

- When the last subsection of a section is requested
  then also the part of the next section (in the same chapter)
  until the start of its first subsection is shown;
  apparently the sections themselves have no entry numbers
  in the sense of `HELP_BOOK_HANDLER.(book.handler).HelpData`.

  (An example is `SizeScreen`.)

- Some pieces of GAP help are very long,
  an example is the entry for `Size` in the HAP package.

On the other hand, duplicate entries are filtered out,
which should perhaps also done in GAP's help system.
The tests cover the situation where one asks for
help about a topic, and where an exact match is found
or just partial matches are found or no matches are found at all.

There is code in `helpstring.g` which cannot be reached via these requests,
such as requests for the next or previous topic in a manual
(which do not make sense in the current setup for Julia),
but I wanted to have a complete "string returning" variant
of GAP's `helpbase.gi`.
- fixed the computation of the end lines of subsections

- changed input and output format of `HELP_String`

- added more tests
- If a `.txt` file is missing then the `HelpData` result has no
  component `start`.

- Some entries in `book.entries` store strings in their first position,
  not numbers; be aware of this when navigating to the next entry.

- If there is exactly one exact match then do not suppress partial matches.
  (This does not happen inside GAP because partial matches are joined with
  the exact matches; but then one cannot restrict the search to exact matches.)
@fingolfin fingolfin merged commit ee7cd01 into oscar-system:master Jul 9, 2019
@wbhart wbhart mentioned this pull request Sep 25, 2019
48 tasks
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