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 of processor and metadata in notebooks #395

Merged
merged 9 commits into from
May 13, 2024

Conversation

zain-sohail
Copy link
Member

@zain-sohail zain-sohail commented May 5, 2024

This PR introduces an expandable representation of the data and metadata, and perhaps plots. We can discuss what should be included and what not.
Just type sp in a notebook after processor class has been instantiated and it will output the html version.

But also the metadata repr is interesting:
To test: you can write sp.attributes in a notebook and that will give the html representation.
If print(sp.attributes) is used, it gives a yaml representation, which is also more human readable than json.

closes #35

@zain-sohail
Copy link
Member Author

Similar additions were attempted in #305 but those are more specific to loaders with runs and maybe to Flash. We can see how to include those.

@coveralls
Copy link
Collaborator

coveralls commented May 5, 2024

Pull Request Test Coverage Report for Build 8987621591

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 86 of 98 (87.76%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 91.704%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sed/core/metadata.py 26 27 96.3%
sed/core/processor.py 5 16 31.25%
Totals Coverage Status
Change from base Build 8920863876: 0.2%
Covered Lines: 6113
Relevant Lines: 6666

💛 - Coveralls

Copy link
Member

@rettigl rettigl left a comment

Choose a reason for hiding this comment

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

I generally like this a lot, however computing the head takes quite some time apparently, making the output quite slow.
Maybe we should discuss which parts of the beamtime addition to the representation we want to add.
Also, we should have tests for this as well (to see that it runs correctly for the different loaders, without a dataframe loaded, etc.).

@rettigl
Copy link
Member

rettigl commented May 5, 2024

Part of the long processing time apparently comes from the metadata conversion:
grafik
This is probably due to large arrays being converted into html, so we should maybe exclude these and replace them with an information about the array shape or so.

@rettigl
Copy link
Member

rettigl commented May 5, 2024

I'd also suggest to move the first layer of metadata one inset in already:
grafik
Because the expandable elements then look like first-layer elements. Maybe generally for all entries (also the dataframe etc.).

@rettigl
Copy link
Member

rettigl commented May 5, 2024

Part of the long processing time apparently comes from the metadata conversion: grafik This is probably due to large arrays being converted into html, so we should maybe exclude these and replace them with an information about the array shape or so.

It mostly is due to the large deformation fields:
grafik

@zain-sohail
Copy link
Member Author

zain-sohail commented May 7, 2024

Part of the long processing time apparently comes from the metadata conversion:
This is probably due to large arrays being converted into html, so we should maybe exclude these and replace them with an information about the array shape or so.

Seems to not be the case. Even with shape, it is slow.

I tried to check directly with a test class and it outputs the html in 0 s
image

I am trying to figure out where the slowdown is.

@zain-sohail
Copy link
Member Author

The problem has been resolved. It was the __repr__ dumping to yaml that took so long. And somehow when _repr_html is called, it also calls __repr__

@zain-sohail
Copy link
Member Author

we should have tests for this as well (to see that it runs correctly for the different loaders, without a dataframe loaded, etc.).

I have added tests to the Metadata class and it's repr/repr_html but not sure how to test what you suggested.

Copy link
Member

@rettigl rettigl left a comment

Choose a reason for hiding this comment

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

LGTM now.
We can still add part of Steinn's code, or do this in a later PR (add issue then).

@zain-sohail zain-sohail merged commit ed38cb1 into main May 13, 2024
6 checks passed
@zain-sohail zain-sohail deleted the repr-html branch May 13, 2024 10:32
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.

Add pretty print for the metadata container (dict)
3 participants