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

Added photosphere_scale param to RPacketPlotter plot #2285

Closed

Conversation

MariamH78
Copy link

📝 Description

Type: 🪲 bugfix

closes #2116 .
I was advised to open a PR here as well so that my modifications can be reviewed by the visualization team, so I'll just copy the content of my PR on jayant's repository.

I added a few functions to the main file, please review them because at some point I confused myself with the math. [I later reviewed the math I used to scale the points' coordinates and I think it's okay, but a second opinion is still needed.]

I added a grid parameter as well, since it makes it easier to view how non-linear the grid is. (It's important to note that I never quite figured out how to configure the scales for the axes correctly, so the values on the axes are approximations at best. Maybe delete the grid_on param since it highlights this flaw? Users should rely on the hovertext values instead, but I also want to somehow highlight the non-linearity of the scale: maybe have a small printed message stating that the axes' scale is not linear if the photosphere_scale is set to anything but 1, and avoid gridlines altogether?)

I updated the docs' notebook as well.

📌 Resources

image

🚦 Testing

How did you test these changes?

  • Testing pipeline
  • Other method (Ran it in a notebook with varying values for photosphere_scale and grid_on)
  • My changes can't be tested (explain why)

☑️ Checklist

  • I requested two reviewers for this pull request
  • I updated the documentation according to my changes
  • I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jamesgillanders
Copy link
Member

Closing this for now. If you are still interested in this PR, please let us know, and we can meet to discuss a slightly different implementation

@MariamH78
Copy link
Author

A bit late, but I would love to hear your feedback and learn more about what would be changed.
Definitely interested in working on it.

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.

reduce the size of photosphere in the RPacketPlotter Plot
4 participants