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

Area definition html representation for Jupyter notebooks #450

Merged
merged 64 commits into from
Dec 13, 2023

Conversation

BENR0
Copy link
Contributor

@BENR0 BENR0 commented Aug 5, 2022

This adds _repr_html_ to the AreaDefinition class. In Jupyter notebooks therefore instead of a string a nicely formatted text is show together with a map overview (cartopy plot with borders and coastlines) as can be seen in the example below. The map by default is folded right now to save space (for the screenshot I unfolded it).

pyresample_area_html_repr

The way this is setup is heavily influenced by how xarray implements the _repr_html_. Basically I used the same directory/ file structure. I also copied some code (the html with the svg icons, the function to read the static data) which I did not attribute yet.

While functionally ready some considerations before this get merged:

  • Apart from some refactoring of the plotting function (removing unecessary code and comments) I was thinking about maybe moving the plotting function into the class?

  • Only the AreaDefinition has a html representation as of now. It would be nice if something similar could be done for SwathDefinition to make the look and feel more homogeneous.

  • Tests added

  • Tests passed

  • Passes git diff origin/main **/*py | flake8 --diff

  • Fully documented

@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (e82bf47) 94.06% compared to head (5191248) 94.23%.
Report is 2 commits behind head on main.

Files Patch % Lines
pyresample/_formatting_html.py 94.57% 7 Missing ⚠️
pyresample/area_config.py 92.30% 1 Missing ⚠️
pyresample/geometry.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #450      +/-   ##
==========================================
+ Coverage   94.06%   94.23%   +0.17%     
==========================================
  Files          85       87       +2     
  Lines       13250    13458     +208     
==========================================
+ Hits        12463    12682     +219     
+ Misses        787      776      -11     
Flag Coverage Δ
unittests 94.23% <95.73%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@coveralls
Copy link

coveralls commented Aug 5, 2022

Coverage Status

coverage: 93.822% (+0.2%) from 93.644%
when pulling 5191248 on BENR0:feat_html_repr
into e82bf47 on pytroll:main.

@djhoese
Copy link
Member

djhoese commented Aug 5, 2022

You may want to try installing the pre-commit hooks. It looks like you've made the style checkers very angry 😉

Other than the style issues, what happens if matplotlib and/or cartopy are not installed? These are not hard requirements of pyresample so we should still show something useful if they are not installed.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

In addition to my previous comment, I think you need to make sure that all of these new non-python files are in the MANIFEST.in so they are included in the distributed package. Lastly, would it be possible to just include the .css and .html files as strings in the new formatting_html.py module? And can we prefix formatting_html.py with an underscore so users are tempted to look into it?

This is really cool. Thanks for putting in the time to get it working.

@BENR0
Copy link
Contributor Author

BENR0 commented Aug 5, 2022

@djhoese Yeah I already saw it :-D. I think most of the complaints are in the plotting which I need to refactor a little anyway.

Good point about matplotlib/cartopy didn't think about that. But in that case I think it should just fall back to the normal string representation.

I will also Look into the other suggestions you made.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

A couple suggestions and questions, but otherwise coveralls is still saying the SwathDefinition branch of the formatting is not covered. That and the PNG output option. I see mention of swath definitions in your test so any idea why those branches aren't being covered?

Comment on lines 28 to 32
try:
import cartopy
cart = True
except ModuleNotFoundError:
cart = False
Copy link
Member

Choose a reason for hiding this comment

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

This is a personal preference, but you could avoid the extra boolean variable (here and for the xarray case) and do cartopy = None in the except case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to change it?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. It's just me being nitpicky.


try:
import xarray as xr
from xarray.core.formatting_html import _obj_repr, datavar_section
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to use a private function (_obj_repr)? How does it differ from repr(xarr_obj)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately yes. repr(xarr_obj) produces the string representation which is also used as a fallback/ in the non notebook case. I could just "copy" the function into pyresample and make it non private?

Copy link
Member

Choose a reason for hiding this comment

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

And your usage differs from the public array_repr and dataset_repr in that same formatting_html module in xarray (these functions use _obj_repr)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because I want to be able to customize the header and the displayed sections (I only want to display the data variables from the whole xarray.Dataset representation). Obviously that is a (personal) design choice which I am happy to talk about. I just tried to reuse as much as was already available but we could also implement something of our own.

Just to give you an idea of the differences (top: _obj_repr, bottom: dataset_repr):
repr_w_obj_repr
repr_w_dataset_repr

Copy link
Member

Choose a reason for hiding this comment

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

Very interesting. Yeah I see what you mean. I was going to say maybe we copy that functionality here, but then noticed it is loading the static icon SVGs so that gets awkward too. I guess that function is small too but it feels weird to copy them.

@mraspaud @pnuu any opinions on this for what "feels" right from the above screenshots? The bottom has so much extra "cruft" for things that aren't used, but maybe that is OK since we are just completely depending on upstream xarray's "private" functions. Or... @BENR0 what about a PR to xarray (in the long term) that let's you exclude empty sections? That would still include the Attributes but maybe that's a good thing? I could see it being beneficial to users to see the extra metadata hanging around the swath definition.

Copy link
Member

Choose a reason for hiding this comment

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

Using a private function/method from another libraries feels wrong I have to say. I agree that the bottom is maybe too verbose, but having the dimension size explicitly here I think is nice actually.

@BENR0
Copy link
Contributor Author

BENR0 commented Oct 5, 2023

A couple suggestions and questions, but otherwise coveralls is still saying the SwathDefinition branch of the formatting is not covered. That and the PNG output option. I see mention of swath definitions in your test so any idea why those branches aren't being covered?

I think the SwathDefinition tests just test the html code part and not the overview plot that's why that branch in the plotting function is not covered. I will try and add tests for the plotting function itself or update the test to also cover the SwathDefinition branch. The PNG output option could be removed I just left it there as a feature but it's not used in the html representation. Should I remove it then or add a test (even though I am not sure how write a check for that yet :-D).

@djhoese
Copy link
Member

djhoese commented Oct 5, 2023

If the PNG stuff isn't used in the final output then I guess I'm ok with it being removed. Although...it might be nice to export an area definition to a PNG. A test wouldn't be too bad as long as you just checked that it was saving and maybe check that it isn't all black. I guess it is up to you what is best for the future. Or maybe @mraspaud has an opinion.

@BENR0
Copy link
Contributor Author

BENR0 commented Oct 9, 2023

The problem with the testing of the plotting function is that I actually don't save the plot to disc but generate the string representation to include it in the html later on. I guess I could check if savefig was called which would be somehow equivalent to checking that something gets saved. I have no idea how to check if the output is all black or not.
/

@djhoese
Copy link
Member

djhoese commented Oct 9, 2023

Given that we're basically depending on another program to output the PNG representation I am OK with just knowing that it isn't empty (0 size).

@djhoese
Copy link
Member

djhoese commented Oct 9, 2023

Ah I see you added specific tests that check savefig.

Comment on lines +48 to +49
@lru_cache(None)
def _load_static_files():
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason for this not to be 1?

Comment on lines 866 to 871

Args:
area_file (str): Path to area yaml file.

Returns:
str: rst list formatted string.
Copy link
Member

Choose a reason for hiding this comment

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

For future reference I think I'd like pyresample to migrate to type annotations and no type information in the docstring.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

A couple more comments, but otherwise I approve of this as long as @pnuu and @mraspaud are OK with that comment about importing private functions from xarray.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM. I think the import of the private function from xarray is indeed fragile, as there is no guaranty of that function's future. However, is seems to be something that is helpful, and I would thus suggest opening an issue in xarray to make this function public instead.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM!

@mraspaud
Copy link
Member

pre-commit.ci autofix

@mraspaud mraspaud merged commit b014586 into pytroll:main Dec 13, 2023
24 of 25 checks passed
@BENR0
Copy link
Contributor Author

BENR0 commented Dec 14, 2023

Opened an issue about the private _obj_repr function here: pydata/xarray#8551

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants