Skip to content

Optional HTML representations for Jupyter Notebook #743

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

Merged
merged 27 commits into from
May 13, 2022

Conversation

DahnJ
Copy link
Contributor

@DahnJ DahnJ commented Feb 3, 2022

Related Issue(s): #508

Description

This is a second attempt (after #573) on providing rich HTML representations for (Jupyter) notebooks.

There are still many areas that should/could be improved, but I'm submitting the PR now for feedback. Any feedback is most welcomed, I'd be happy to align this with expectations and requirements from the PySTAC developers/community.

Some areas of possible improvement in this or a subsequent PR

  • Catalog vs Collection (some fields such as extent are missing from collections since the template simply uses extra_fields)
  • Additional templates / formatting, e.g. for providers, properties
  • Providing some support for extensions (e.g. would be nice to have pretty-formatted eo:bands )

Examples

To reproduce the examples, use the example catalog

## Execute in a notebook
import pystac
cat = pystac.Catalog.from_file('./pystac/docs/example-catalog/catalog.json')
cat

Catalog shows children

image

Collection

image

Item with Assets

image

Changes from #573

Based on #573 with the following improvements:

  • HTML representation is now completely optional. If Jinja isn't installed, repr is used.
  • Catalogs show their children
  • Items show their assets
  • Templates refactored and improved upon

PR Checklist

  • Code is formatted (run pre-commit run --all-files)
  • Tests pass (run scripts/test)
  • Documentation has been updated to reflect changes, if applicable
  • This PR maintains or improves overall codebase code coverage.
  • Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

DahnJ added 3 commits February 3, 2022 00:58
Starting point is stac-utils#573
Improvements
- Jinja dependency is optional, otherwise __repr__ is used
- Catalog shows children
- get_items() is kept lazy
- Minor improvements in the Jinja templates
- Use macros to simplify Jinja templates
- Assets show title before expansion
Copy link
Collaborator

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @DahnJ! These reprs looks great.

Copy link
Contributor

@duckontheweb duckontheweb left a comment

Choose a reason for hiding this comment

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

Thanks for this @DahnJ! I really like the way it's organized; the templates are very understandable, and it's easy to trace a PySTAC class to its template.

Regarding some of the possible "points of improvement" in your description...

  • Catalog vs Collection (some fields such as extent are missing from collections since the template simply uses extra_fields)

See my review comments, but I think it makes sense to break these into separate templates. I think that would make it easier to handle some of the extra fields in Collection (like extent).

  • Additional templates / formatting, e.g. for providers, properties
  • Providing some support for extensions (e.g. would be nice to have pretty-formatted eo:bands )

I agree it would be good to flesh out some of the other classes like Provider and the extension objects.

Should we also add a jinja2 extra here? In general, users would probably be using this functionality in a Jupyter notebook or other environment where jinja2 is already installed, but it might be nice to give them the option to install it as an extra as well.

@duckontheweb
Copy link
Contributor

@DahnJ Sorry for the long silence on this, I'm just coming back to this now. I just updated the branch to reflect the latest changes to main. This looks good to me after the change to only list the first Item in the Catalog/Collection summaries. There is a lint error (black is reformatting one of the files), but aside from that it looks ready to me.

@codecov-commenter
Copy link

codecov-commenter commented May 4, 2022

Codecov Report

Merging #743 (2137ff0) into main (008d83c) will decrease coverage by 0.33%.
The diff coverage is 38.57%.

@@            Coverage Diff             @@
##             main     #743      +/-   ##
==========================================
- Coverage   94.64%   94.31%   -0.34%     
==========================================
  Files          81       83       +2     
  Lines       11803    11873      +70     
  Branches     1378     1387       +9     
==========================================
+ Hits        11171    11198      +27     
- Misses        453      496      +43     
  Partials      179      179              
Impacted Files Coverage Δ
pystac/html/jinja_env.py 33.33% <33.33%> (ø)
pystac/asset.py 84.28% <37.50%> (-6.04%) ⬇️
pystac/catalog.py 91.10% <37.50%> (-1.06%) ⬇️
pystac/collection.py 92.46% <37.50%> (-1.81%) ⬇️
pystac/item.py 88.88% <37.50%> (-2.17%) ⬇️
pystac/item_collection.py 87.50% <37.50%> (-6.25%) ⬇️
pystac/link.py 88.60% <37.50%> (-2.21%) ⬇️
pystac/provider.py 79.54% <37.50%> (-9.35%) ⬇️
pystac/html/__init__.py 100.00% <100.00%> (ø)

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 008d83c...2137ff0. Read the comment docs.

@duckontheweb
Copy link
Contributor

It looks like we'll also need to configure mypy to ignore jinja2 imports as well (similar to this line for setuptools.

@duckontheweb duckontheweb requested a review from gadomski May 4, 2022 13:22
@duckontheweb duckontheweb added this to the 1.5.0 milestone May 4, 2022
@TomAugspurger
Copy link
Collaborator

Lazy question: does this handle pystac.ItemCollection too? If not, it doesn't need to hold up this PR but I think it'd be great to do as a followup.

gadomski and others added 3 commits May 11, 2022 11:59
jinja2 is used for optional html_repr for Jupyter Notebooks
and should thus not be required
@DahnJ
Copy link
Contributor Author

DahnJ commented May 11, 2022

It looks like we'll also need to configure mypy to ignore jinja2 imports as well (similar to this line for setuptools.

Done in 6cf3fa4

@DahnJ
Copy link
Contributor Author

DahnJ commented May 11, 2022

Lazy question: does this handle pystac.ItemCollection too? If not, it doesn't need to hold up this PR but I think it'd be great to do as a followup.

I will still have a quick look at this tomorrow, but after that this is good to go from my side 👍

@DahnJ
Copy link
Contributor Author

DahnJ commented May 12, 2022

Lazy question: does this handle pystac.ItemCollection too? If not, it doesn't need to hold up this PR but I think it'd be great to do as a follow-up.

I have implemented a rudimentary template for ItemCollection in 4328a77 which simply lists items and extra_fields (if present). To my understanding, there's not much more to ItemCollection than that.

This is what it looks like

image

However, I have shown all the items. If I understood it correctly, ItemCollection is simply a GeoJSON FeatureCollection and thus cannot end up fetching the items over the network? If I misunderstood, then I can simply show the first Item only as with Catalog/Collection.

cc: @duckontheweb

@TomAugspurger
Copy link
Collaborator

TomAugspurger commented May 12, 2022

Great!

However, I have shown all the items. If I understood it correctly, ItemCollection is simply a GeoJSON FeatureCollection and thus cannot end up fetching the items over the network?

I think that's right.

We might want to limit the ItemCollection repr to some number of items (maybe 10?). It's common to have an ItemCollection with hundreds or thousands of items.

@duckontheweb
Copy link
Contributor

If I understood it correctly, ItemCollection is simply a GeoJSON FeatureCollection and thus cannot end up fetching the items over the network?

Yes, this is correct. These Items will already be resolved in memory.

We might want to limit the ItemCollection repr to some number of items (maybe 10?). It's common to have an ItemCollection with hundreds or thousands of items.

I agree with @TomAugspurger on this, I think it would be good to limit the number of Items displayed.

ItemCollection could theoretically have hundreds or thousands of items
which shouldn't all be shown in the HTML representation at once
@DahnJ
Copy link
Contributor Author

DahnJ commented May 12, 2022

We might want to limit the ItemCollection repr to some number of items (maybe 10?). It's common to have an ItemCollection with hundreds or thousands of items.

I agree with @TomAugspurger on this, I think it would be good to limit the number of Items displayed.

Okay, I pushed 2137ff0 with the changes:

If there are <=10 items, the behaviour stays the same:

image

But if there are over 10 items, only the first 10 are shown with a helpful message:

image

Copy link
Collaborator

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

Copy link
Contributor

@duckontheweb duckontheweb left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with this one @DahnJ, this is a pretty big feature add to the library!

@duckontheweb duckontheweb merged commit 3526b65 into stac-utils:main May 13, 2022
DahnJ added a commit to DahnJ/pystac that referenced this pull request May 13, 2022
@gadomski gadomski mentioned this pull request Feb 15, 2023
5 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.

5 participants