-
-
Notifications
You must be signed in to change notification settings - Fork 426
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
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…-mc-packet-visualization
…ntbhakar/tardis into add-mc-packet-visualization
Comments were removed by mistake, added them back in
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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 |
A bit late, but I would love to hear your feedback and learn more about what would be changed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📝 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 thehovertext
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 thephotosphere_scale
is set to anything but 1, and avoid gridlines altogether?)I updated the docs' notebook as well.
📌 Resources
🚦 Testing
How did you test these changes?
photosphere_scale
andgrid_on
)☑️ Checklist
build_docs
label