the-virtual-brain / tvb-root

Main TVB codebase
https://thevirtualbrain.org
Other
121 stars 102 forks source link

WID-14 Improve the HTML representation on a Traited obj in Jupyter lab #639

Closed rarescodemart closed 1 year ago

rarescodemart commented 1 year ago

I converted the description for each model from RST format to HTML and I managed to render the mathematical equations (which are present in it) using MathJax.

liadomide commented 1 year ago

The result would look as in the attached image

Screenshot 2023-01-11 at 21 59 27
i-Zaak commented 1 year ago

The result would look as in the attached image Screenshot 2023-01-11 at 21 59 27

This looks great! However is there a reason to duplicate the class docstrings in the dfun?

liadomide commented 1 year ago

This looks great! However is there a reason to duplicate the class docstrings in the dfun?

dfun.__doc__ is expected to have only the equations (plus minimum text in some cases). The Class docstring is more generic and the eq there help for the overall description of the Model.

It is a decision we made a long time ago, to keep the eq in both places. Some places which have less space to display (here, the PhasePlane in tvb web GUI) are better if only the equations are given. The overall Sphinx generated documentation can benefit for the class extended documentation.

It might be that we need to revise this decision... For the current task we only took the existent convention/state and worked on the display in jupyterlab ;-)

i-Zaak commented 1 year ago

I see, wasn't aware of that - in that case it of course makes sense. I guess the class docstring then would be shown for the component instance representation, to render e.g. the Coupling equations.

Alternatively one could keep the simulator representation as is (no equations) and show more details for the individual components separately.

i-Zaak commented 1 year ago

And one more thing - do you want to do some more improvements for the HTML representations in this PR, or just focus on the equations?

liadomide commented 1 year ago

And one more thing - do you want to do some more improvements for the HTML representations in this PR, or just focus on the equations?

Very good question. Could we delay answering until our next call 17 Jan? Then we can have a small demo with the current state, and we decide how to proceed... Then, we would not merge until after 17th

i-Zaak commented 1 year ago

Could we delay answering until our next call 17 Jan?

Of course. In the meantime, these might be relevant in the long-term:

liadomide commented 1 year ago

Alternatively one could keep the simulator representation as is (no equations) and show more details for the individual components separately.

That might be a more generic strategy to follow (instead of the current code in this PR). Because on Model we have dfun._doc__ (as used here), but Coupling has only an equation on the class doc (in this PR not shown)

liadomide commented 1 year ago

With only one dependency in tvb.basic.neotraits.info towards docutils, we get the following on instance level:

For a model instance : image

A coupling instance: image

And the simulator: image

@i-Zaak would this outcome be ok ?

i-Zaak commented 1 year ago

@i-Zaak would this outcome be ok ?

These look great!

i-Zaak commented 1 year ago

I guess the next step would be to fix the representation of the monitors instances and the connectivity, correct?

liadomide commented 1 year ago

I guess the next step would be to fix the representation of the monitors instances and the connectivity, correct?

image

image

i-Zaak commented 1 year ago

image

And the connectivity breaks when instantiated like this:

image

liadomide commented 1 year ago

And the connectivity breaks when instantiated like this:

Thank you @i-Zaak for testing. I fixed the HTML/summary_info result for "empty" Connectivity and Surface (that was similarly broken).

For the Bold monitor, the doc string on the class (having a table of papers) is clashing with our html table for the attributes. I leave that to @rarescodemart on Monday to find a good compromise.

i-Zaak commented 1 year ago

Hello, just had a quick look - looks great to me across the components. Only before it was also working in the VSCode (here on codespaces), whereas now the equations don't render anymore. I personally don't care that much, but it was a nice-have...

liadomide commented 1 year ago

before it was also working in the VSCode (here on codespaces), whereas now the equations don't render anymore

Could you put a before & after screenshot, so that we know which parts are affected? Or, can you identify if it was the docstrings small fixes that produced this effect, or was it the HTML representation ?

i-Zaak commented 1 year ago

Could you put a before & after screenshot, so that we know which parts are affected?

Current:

image

The before screenshot is difficult to do as after the rebases I'm not able to import the tvb library on the previous commits in the Codespace due to some TVB profile errors. I don't have a local install of the VS studio to try it out.

image

liadomide commented 1 year ago

@i-Zaak thanks for this feedback.

We reported the bug TVB-3067 for the Profile issue you mention above. It was recently solved in PR #643.

Codespaces is showing some equations, if we use the intermediate HTML representation generated with rst2html library (commit #580a54fbe0bfd7a7903c79c3f02883271628ce28 "try html2rst"). Unfortunately the code version from that commit does not render all the equations correctly (for example it does not support the complex case, if then situations - and we have many in models). The latest code (based on docutils technology) is not functioning in Codespaces because MathJax does not exist in the front-end IDE (but MathJax exists by default in JupyterLab)

And this a risk of the current implementation: we rely on the outside (jupyterlab) to have MathJax. In what I consider very low chances that JupyterLab drops MathJax, we need to find another solution (or don't display equations - like before).

Considering the above, I would still support that we merge the current form, as still being an improvement to the prev state.