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

Refactor visualizers package to improve separation between package specific visualizers and reusable visualizers #23

Merged
merged 8 commits into from
Sep 20, 2024

Conversation

ncguilbeault
Copy link
Collaborator

Summary

This PR refactors the visualizers package to enhance modularity and maintainability. This update introduces a clear separation between package-specific visualizers (e.g. Bonsai.ML.Visualizers.LinearDynamicalSystems) and reusable visualizer utilities/base classes (contained within the core Bonsai.ML.Visualizers). This improves the user experience for those who only wish to use a single Bonsai.ML package with it's associated visualizers, and speeds up/lightens the installation process by not having to reference all Bonsai.ML packages within the core Bonsai.ML.Visualizers package.

@ncguilbeault ncguilbeault force-pushed the visualizers-refactor branch 3 times, most recently from ed168f4 to 0d8a2eb Compare July 16, 2024 14:19
@ncguilbeault ncguilbeault force-pushed the visualizers-refactor branch 3 times, most recently from 1cec736 to 819137a Compare September 5, 2024 10:25
Copy link
Member

@glopesdev glopesdev left a comment

Choose a reason for hiding this comment

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

Everything looks good in general, my main question is whether we will ever only have visualizers in these packages, or if we will have editors or other visual configuration controls as well.

In general we use the suffix .Design for similar packages, e.g. Bonsai.Vision.Design which include both visualizers and interactive editors. In this case the naming would be Bonsai.ML.LinearDynamicalSystems.Design and Bonsai.ML.HiddenMarkovModels.Design if we go for the same standard.

src/Bonsai.ML.Visualizers/EllipseHelper.cs Outdated Show resolved Hide resolved
src/Bonsai.ML.Visualizers/EllipseHelper.cs Outdated Show resolved Hide resolved
src/Bonsai.ML.Visualizers/EllipseHelper.cs Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Member

@glopesdev glopesdev left a comment

Choose a reason for hiding this comment

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

Just adding a few typos / minor suggestions, but otherwise LGTM. In the GitHub actions output (actions/runs/10740426883) there seems to be a few minor XML doc strings that maybe you could add, but otherwise I'm happy to merge this.

README.md Outdated Show resolved Hide resolved
@ncguilbeault ncguilbeault merged commit cdcc7c2 into main Sep 20, 2024
2 checks passed
@glopesdev glopesdev deleted the visualizers-refactor branch September 20, 2024 15:16
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