Skip to content

place main viewplot in the center #178

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

whelena
Copy link
Collaborator

@whelena whelena commented Apr 14, 2025

Description

Original: main clone vp is set so that the top is aligned with y = 0.9. This works great with default plotting.direction = 'down', but not the other direction. Setting back to defaults, which works better

Small example

Top = before, bottom = after
Screenshot 2025-04-14 at 11 50 16 AM

Larger example

Left = before, right = after
Screenshot 2025-04-14 at 12 00 37 PM

Checklist

  • This PR does NOT contain Protected Health Information (PHI). A repo may need to be deleted if such data is uploaded.
    Disclosing PHI is a major problem1 - Even a small leak can be costly2.

  • This PR does NOT contain germline genetic data3, RNA-Seq, DNA methylation, microbiome or other molecular data4.

  • This PR does NOT contain other non-plain text files, such as: compressed files, images (e.g. .png, .jpeg), .pdf, .RData, .xlsx, .doc, .ppt, or other output files.

  To automatically exclude such files using a .gitignore file, see here for example.

  • I have read the code review guidelines and the code review best practice on GitHub check-list.

  • I have set up or verified the main branch protection rule following the github standards before opening this pull request.

  • The name of the branch is meaningful and well formatted following the standards, using [AD_username (or 5 letters of AD if AD is too long)]-[brief_description_of_branch].

  • I have added the major changes included in this pull request to the CHANGELOG.md under the next release version or unreleased, and updated the date.

Footnotes

  1. UCLA Health reaches $7.5m settlement over 2015 breach of 4.5m patient records

  2. The average healthcare data breach costs $2.2 million, despite the majority of breaches releasing fewer than 500 records.

  3. Genetic information is considered PHI.
    Forensic assays can identify patients with as few as 21 SNPs

  4. RNA-Seq, DNA methylation, microbiome, or other molecular data can be used to predict genotypes (PHI) and reveal a patient's identity.

@whelena
Copy link
Collaborator Author

whelena commented Apr 14, 2025

this is an improvement but obviously not ideal, I thought of two approaches that might work:

  1. Change the alignment on the viewport based on the plotting.direction, defaulting to centered when an angle/number is used to avoid complication.
  2. In the save.plot function, draw the grob inside a viewport that can be moved around using user defined params. Want to avoid this since it puts to much responsibility on the user (not user-friendly)

I think these two approaches are similar, would benefit from someone who understands viewports better - @dan-knight

@whelena whelena requested a review from dan-knight April 14, 2025 19:06
@dan-knight
Copy link
Collaborator

I think we could refine this a tiny bit. All plotting directions are just an angle under the hood. For example, right is just a convenient way to set the plotting angle to 90. I think the start could simply rotate around the center based on the underlying plotting.direction number. Then, down/0 would be unchanged, but other plotting directions would be adjusted accordingly.

@whelena
Copy link
Collaborator Author

whelena commented Apr 16, 2025

Ya we could try that, what about weirder angles like 45 or 60? SHould we just collapse them to the nearest direction?

@dan-knight
Copy link
Collaborator

Ya we could try that, what about weirder angles like 45 or 60? SHould we just collapse them to the nearest direction?

Still works! Imagine a circle around the plot center point. The starting node would just rotate continuously along that circle with whatever plotting direction angle is specfied (45, 60, 31, 0.00151, anything).

@whelena
Copy link
Collaborator Author

whelena commented Apr 18, 2025

Shifted the plot downward in calculate.main.plot.size to ensure all content is visible. Ideally, we'd set yscale = c(ymax, ymin) in the make.plot.viewport function (currently unsupported). Additionally, doing that distorts other plot component (normal node placement). I think it's due to how viewports are layered throughout.

I propose we mark this as a potential future fix for cleaner scaling and merge this after fixing the tests?

Screenshot 2025-04-18 at 2 14 28 PM

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