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

visualizer interface #1062

Open
wants to merge 1 commit into
base: robynpy_release
Choose a base branch
from

Conversation

shivkanthb
Copy link

Project Robyn

This PR adds a new visualization module to the Python port of Robyn. The change introduces a RobynVisualizer class with methods that correspond to the various plotting functions used in the R version of Robyn. This new module will enable the Python version to generate similar visualizations as the R version, enhancing the functionality and user experience of the Python port.

Fixes #[Issue number for Python port visualization]

Type of change

  • feat: New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Fork the repo and create your branch from master (latest dev version).
  • The new RobynVisualizer class has been added with method declarations that mirror the R version's plotting functions.
  • No actual implementation has been added yet, so full testing will be required once the methods are implemented.
  • Ensure all existing tests pass after adding this new module.
  • Make sure the new code follows the project's coding standards and lints properly.

Note: This PR only includes the structure for the visualization module. Actual implementation of the plotting functions will be needed in subsequent PRs. Documentation updates will also be required once the implementation is complete.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 18, 2024
@shivkanthb
Copy link
Author

This is not fully ready it but more of a v0 scaffold

Copy link

@sumane81 sumane81 left a comment

Choose a reason for hiding this comment

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

module needs to broken down into smaller, relevant plotters

Choose a reason for hiding this comment

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

Is this single visualizer class for all the plots?
As discussed, we should be following single responsibility principle here. all plots plotted using input data vs modeling vs allocator etc should reside in it's own classes with relevant data received in init method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants