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 __repr__ for Sorting objects #2747

Merged
merged 5 commits into from
May 13, 2024

Conversation

h-mayorquin
Copy link
Collaborator

@h-mayorquin h-mayorquin commented Apr 23, 2024

I was working on html representation last week https://ipython.readthedocs.io/en/stable/config/integrating.html and I realize there is low hanging fruit for using them in our objects. This is an example for the sorting objects. If this works and you guys agree we can create a similar one for the recording objects. Also, it would be good to include general annotations.

Any suggestions are welcome

This is how it looks:

Screencast.from.2024-04-22.12-35-30.webm

@h-mayorquin h-mayorquin self-assigned this Apr 23, 2024
@zm711
Copy link
Collaborator

zm711 commented Apr 24, 2024

I think this looks super cool, but as someone who does not know HTML at all I wonder what the developer burden would be for this? I definitely couldn't help with this if we add html in at this point, but if everyone else knows HTML then I think the usefulness factor would outweigh the development concerns.

@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Apr 24, 2024

I think this is a good consideration. Two points to add to the estimate of developer burden:

  1. The current normal __repr__ hasn't been changed that much. It changed it once when we improved it to display multisegment information, time and memory and that's it. No other maintenance other than the original feature development. I think that is a good reference class for the html repr, ergo, I don't expect maintenance burden to be that high.
  2. It is less than 10 lines with only html. No css, no javascript. I expect that I can explain this in less to 5 minutes to someone that never did html. Maybe we could do the excercise in a short call to see if I am right? I should also probably add a docstring but let's see first what the others say or suggest.

@JoeZiminski
Copy link
Collaborator

This is cool, a quick suggestion for maintainability would be to include functions that generate html representations in a separate module. This __repr__ code looks reasonably generic (?) such that many different classes could be represented with it.

Maybe the pattern could be something like:

def __repr__html(self):
    return utils.get_html_repr("basesorting", self)

(utils as a generic placeholder name).

The three benfits I see from this are:

  1. all html code is in the same place, this is handy when making generic html repr improvements rather than having to search for all __repr__html across the codebase.
  2. avoids mixing a lot of python vs. html code in the same place
  3. Makes it easier if the code that generates the html repr can be made generic across classes.

@zm711 zm711 added the enhancement New feature or request label Apr 25, 2024
@h-mayorquin
Copy link
Collaborator Author

@JoeZiminski

This is cool, a quick suggestion for maintainability would be to include functions that generate html representations in a separate module. This repr code looks reasonably generic (?) such that many different classes could be represented with it.

I would like to wait till we do the SortingAnalyser representation to generalize and abstract. Knowing my personal tendencies I am more prone to generalizing too early and overthinking rather than uneceesarily duplicating code. Once we have three examples (sorting, recording and sortinganalyser) I would be glad to work with you on putting this on a module on its own, what do you think?

@alejoe91 alejoe91 merged commit b5b71fb into SpikeInterface:main May 13, 2024
11 checks passed
@JoeZiminski
Copy link
Collaborator

Hey @h-mayorquin sorry for late reply, this makes sense! Let me know when the time is right, would be happy to help.

@h-mayorquin h-mayorquin deleted the add_sorting_html_repr branch May 13, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants