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

Add Log Plot Support to Scatter Plot Visualization Operator #2712

Merged
merged 7 commits into from
Jul 4, 2024

Conversation

sixsage
Copy link
Collaborator

@sixsage sixsage commented Jul 1, 2024

Purpose

Modify the scatter plot operator to support log-scaling in the X and/or Y axis

Description of Changes

As suggested by kunwp1, log plot was originally a visualization operator on its own but decided to merge it to scatter plot operator instead.
The log-scaling arguments were found here

  • ScatterplotOpDesc.scala: implement log-scaling into the original scatter plot operator
    image

@sixsage sixsage self-assigned this Jul 1, 2024
@sixsage sixsage marked this pull request as ready for review July 1, 2024 13:31
@sixsage sixsage requested a review from aglinxinyuan July 1, 2024 13:31
@aglinxinyuan aglinxinyuan requested review from kunwp1 and removed request for aglinxinyuan July 2, 2024 21:42
@kunwp1
Copy link
Collaborator

kunwp1 commented Jul 2, 2024

Hey @sixsage, I wanted to let you know that your PR looks excellent! My only suggestion is that the plot appears similar to the existing scatter plot. Both use the Plotly API but with different parameters. Instead of creating the log plot as a new visualization operator, it would be nice to integrate your work into the scatter plot. I'm curious to hear your thoughts on this. Thank you.

@sixsage
Copy link
Collaborator Author

sixsage commented Jul 3, 2024

Thank you for bringing that up. I agree that the log plot can be integrated into the scatter plot operator and I will make the change. Should I close this PR and start a new PR when I am done or should I keep this open?

@sixsage sixsage changed the title Add Log Plot Visualization Operator Add Log Plot Support to Scatter Plot Visualization Operator Jul 3, 2024
@sixsage
Copy link
Collaborator Author

sixsage commented Jul 3, 2024

Hello @kunwp1 - I have made the changes you have kindly suggested. If you have the time it would be great if you could review my PR once more. Thank you!

Copy link
Collaborator

@kunwp1 kunwp1 left a comment

Choose a reason for hiding this comment

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

Thank you for the change! I think the PR is in a good shape! You can merge it after addressing the comment.

Copy link
Collaborator

@kunwp1 kunwp1 left a comment

Choose a reason for hiding this comment

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

Thanks! You can now merge it

@sixsage sixsage merged commit 697b0a3 into master Jul 4, 2024
8 checks passed
@sixsage sixsage deleted the kyuho-add-logplot-operator branch July 4, 2024 04:32
aglinxinyuan added a commit that referenced this pull request Sep 26, 2024
**Purpose**:
The purpose of the PR is to solve the issue where the server startup
script won't work properly due to the enormous list of dependencies.

**Description**:
Use java argument file as a workaround to solve the command line
argument too long problem. Added a new bash script template to generate
the startup script.
Fix: #2712

---------

Co-authored-by: Xinyuan Lin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants