Skip to content

feat: support highlight in multilayer and subplots #157

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

Conversation

dakshpokar
Copy link
Contributor

Description

This PR adds support for Highlight in MultiLayer and Subplots in PyMaidr using the newer Maidr TS engine.

Type of Change

  • Bug fix
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

maidr/core/enum/maidr_key.py:8

  • [nitpick] Since the key was updated from 'selector' to 'selectors' to support multiple values, please update corresponding documentation and ensure that all references in the codebase match the new naming.
SELECTOR = "selectors"

maidr/core/context_manager.py:80

  • The switch from contextvars to plain class-level variables for managing elements and selector IDs may introduce thread-safety issues; consider validating that this approach is safe in concurrent environments or document any limitations.
elements = {}

@dakshpokar dakshpokar changed the base branch from main to feat/dodged-stacked-bar-plot April 15, 2025 04:18
@xability xability deleted a comment from Copilot AI Apr 15, 2025
@dakshpokar
Copy link
Contributor Author

dakshpokar commented Apr 15, 2025

Greetings Professor @jooyoungseo

This project was a formidable challenge, but I'm excited to share our breakthrough. After extensive investigation, collaborative discussions with @SaaiVenkat led us to a pivotal discovery: by refining our implementation of the HighlightContextManager and leveraging matplotlib's internal gid parameter, we can now accurately tag the elements required for highlighting in subplots and multilayer plots. Additionally, this approach has paved the way for implementing the innovative highlight methodology we previously discussed.

Currently, I am using UUIDs to identify elements; however, we have the opportunity to optimize further by tagging SVG elements with specific data points (such as (x, y) coordinates). This enhancement would allow the TS engine to dynamically retrieve these elements without the need for sorting or rearranging, streamlining our process significantly.

Note that this PR is a breaking change and would need TS backend BY DEFAULT!! No backwards compatibility whatsoever.

So finally, I would like to say that this completes the migration to TS engine. I think we are ready to shift the switch to TS by default in Environment. For this we need to merge these PRs in this order -> #153 #154 #155 #156 and finally this PR

Thanks,
Daksh Pokar

Copy link
Member

@jooyoungseo jooyoungseo left a comment

Choose a reason for hiding this comment

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

Fantastic! This is a huge enhancement! Thanks a zillion for your hard work, @dakshpokar and @SaaiVenkat !

Base automatically changed from feat/dodged-stacked-bar-plot to feat/maidr-ts-migration-phase-2 April 15, 2025 15:39
@dakshpokar dakshpokar merged commit 58e16b3 into feat/maidr-ts-migration-phase-2 Apr 15, 2025
3 checks passed
@dakshpokar dakshpokar deleted the feat/multilayer-multipanel-highlight branch April 15, 2025 15:43
@dakshpokar dakshpokar mentioned this pull request Apr 28, 2025
10 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.

2 participants