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

HTML representation inside Jupyter notebook #105

Merged
merged 25 commits into from
Nov 24, 2021
Merged

HTML representation inside Jupyter notebook #105

merged 25 commits into from
Nov 24, 2021

Conversation

lang-m
Copy link
Member

@lang-m lang-m commented Oct 11, 2021

No description provided.

@lang-m lang-m marked this pull request as draft October 11, 2021 13:38
@lang-m lang-m linked an issue Oct 11, 2021 that may be closed by this pull request
@lang-m
Copy link
Member Author

lang-m commented Oct 15, 2021

We should merge the other branches into this one before fixing any tests.

@marijanbeg
Copy link
Member

LGTM. Just a brain dump: I wanted to re-think how we generate mif strings in oommfc, which could be related to this. At the moment, we write text in code, which looks ugly and it is easy to make a mistake. A possible solution would be to have templates in files and based on those templates we create mif strings (or in this case HTML representations).

https://docs.python.org/3/library/string.html#template-strings

@fangohr
Copy link
Member

fangohr commented Oct 19, 2021

Nice idea. What does the output look like?

@lang-m
Copy link
Member Author

lang-m commented Oct 20, 2021

grafik

@lang-m
Copy link
Member Author

lang-m commented Oct 20, 2021

Templates for the mif files are definitely a good idea. However, string templates allow for relatively little flexibility. So I would suggest considering using Jinja2 (https://jinja2docs.readthedocs.io/en/stable/) as an alternative which is a lot more flexible. It's primarily targeted at HTML, etc. but I think it should fit our needs as well. I know, this would mean adding an additional dependency. But we are using it (indirectly) for the sphinx documentation already. At least the theme we are using (and maybe sphinx in general) relies on Jinja2 for the HTML templates.

I could create a short example mif template (without any logic to actually fill in anything). That might make it easier to judge.

@marijanbeg
Copy link
Member

I like that. As mentioned, my idea was just a brain dump and low-priority. I will open a separate issue somewhere just to remind us.

@lang-m
Copy link
Member Author

lang-m commented Nov 8, 2021

The output in the notebook (_repr_html_ and __str__) now is:
[edit: typo in region fixed]

  • Region
    grafik

  • Mesh
    grafik

  • Field
    grafik

  • FieldRotator
    grafik

@marijanbeg
Copy link
Member

Very nice!

@lang-m
Copy link
Member Author

lang-m commented Nov 8, 2021

Fourier-transformed field does contain real-space mesh information (in _repr_html_):
grafik

@lang-m
Copy link
Member Author

lang-m commented Nov 8, 2021

@marijanbeg We do currently have tests for __repr__. We should think about how to test the new functionality, i.e. if we write some sort of unit test for both __str__ and _repr_html_ or if we test the HTML representation only inside a Jupyter notebook (if this works properly).

@marijanbeg
Copy link
Member

I think we should call these methods explicitly in tests at least. This way we ensure the coverage does not go down.

@lang-m
Copy link
Member Author

lang-m commented Nov 15, 2021

Implementing __str__ instead of __repr__ is a disadvantage for mesh.subregions:
grafik

@lang-m lang-m marked this pull request as ready for review November 18, 2021 14:01
@lang-m lang-m requested a review from fangohr November 18, 2021 15:05
@lang-m
Copy link
Member Author

lang-m commented Nov 18, 2021

@fangohr Please ignore changes in the notebooks (these almost exclusively related to rerunning the notebooks to reflect changes in representation).

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2021

Codecov Report

Merging #105 (ae0aed0) into master (9e7202e) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
+ Coverage   95.25%   95.36%   +0.10%     
==========================================
  Files          19       20       +1     
  Lines        2004     2029      +25     
==========================================
+ Hits         1909     1935      +26     
+ Misses         95       94       -1     
Impacted Files Coverage Δ
discretisedfield/field.py 97.59% <100.00%> (-0.01%) ⬇️
discretisedfield/field_rotator.py 98.66% <100.00%> (+1.44%) ⬆️
discretisedfield/html/__init__.py 100.00% <100.00%> (ø)
discretisedfield/mesh.py 98.97% <100.00%> (+0.01%) ⬆️
discretisedfield/region.py 98.73% <100.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e7202e...ae0aed0. Read the comment docs.

@fangohr
Copy link
Member

fangohr commented Nov 23, 2021

Thank you very much - this looks great.

I added very minor suggestions. To deal with those, running black, isort (and just to complete the trio) flake8 on the files may be useful. Otherwise, this is all great and fine with me - go ahead and merge you you are ready.

[The template approach is great.]

discretisedfield/html/__init__.py Outdated Show resolved Hide resolved
discretisedfield/mesh.py Show resolved Hide resolved
MANIFEST.in Outdated Show resolved Hide resolved
@lang-m lang-m merged commit 7eb7598 into master Nov 24, 2021
@lang-m lang-m deleted the html_repr branch November 24, 2021 10:55
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.

Representation strings.
4 participants