Skip to content

Added a basic scatterplot for Magick backend #45

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
merged 62 commits into from
Jul 12, 2019

Conversation

alishdipani
Copy link
Member

I Passed the rspec test for scatterplot for Magick backend and set up basic functions for Magick backend.
I modified the Magick backend so that it can:

  • Draw x and y axis
  • Draw markers
  • Draw text with rotation and size
  • Plot a rudimentary scatter plot

Since, GR and Magick backend uses different units for sizes, for now I have multiplied the transform function with a hard-coded value 10. I will find a suitable factor to multiply for a normalized scale for all backends. Also, no ticks are implemented in the x and y axis, I will add ticks in future.

Currently the implemented scatterplot looks like this:

Magick_scatterplot

Please review my changes.

@@ -108,6 +108,9 @@ def write(file_name, device: :file)
Rubyplot.backend.canvas_height = @height
Rubyplot.backend.canvas_width = @width
Rubyplot.backend.figure = self
if ENV["RUBYPLOT_BACKEND"] == "MAGICK"
Copy link
Member

Choose a reason for hiding this comment

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

FInd a way to move this to the backend. I don't want anything backend specific in the front end.

If you feel a compulsive need to do something like this, we should change the whole design and not do such patch work. Such things dont scale.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will try to find a way to add it into the magick backend, one another way would be to add this function to GR backend as well.

Copy link
Member

Choose a reason for hiding this comment

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

I want to merge this PR, however I cannot do it unless you remove the if here. Please resolve this on priority before starting work for this week.

Copy link
Member Author

Choose a reason for hiding this comment

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

[Commit Link]
Done, shifted it to backend, the task is performed during init_output_device

@v0dro
Copy link
Member

v0dro commented May 16, 2019

Code is good overall, but you need to improve this plot. Please proceed with the rest of your project.

allow specifying separate fill and border marker color for markers
@v0dro v0dro merged commit 1b1f765 into SciRuby:master Jul 12, 2019
@v0dro
Copy link
Member

v0dro commented Jul 12, 2019

Have merged. You can start work on ticks and any front end changes that you might need now.

alishdipani added a commit to alishdipani/rubyplot that referenced this pull request Jul 17, 2019
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