tardis-sn / tardis

TARDIS - Temperature And Radiative Diffusion In Supernovae
https://tardis-sn.github.io/tardis
198 stars 403 forks source link

Last Interaction radius Plot #2651

Closed sarthak-dv closed 3 weeks ago

sarthak-dv commented 1 month ago

:pencil: Description

Type: :rocket: feature

This PR is part of the Velocity Packet Tracker Visualisation tool. It introduces a new Python script interaction_radius_plot.py containing the InteractionRadiusPlotter class and a Jupyter notebook interaction_radius_plot.ipynb to demonstrate accessing the class and visualizing the plot. The plots are generated using both Matplotlib and Plotly.

This PR aims to discuss and finalize the look and feel of the Last Interaction Radius plot.

Matplotlib: mpl

Plotly: plotly

:pushpin: Resources

Examples, notebooks, and links to useful references.

:vertical_traffic_light: Testing

How did you test these changes?

:ballot_box_with_check: Checklist

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

review-notebook-app[bot] commented 1 month ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

tardis-bot commented 1 month ago

*beep* *bop*

Hi, human.

The docs workflow has succeeded :heavy_check_mark:

Click here to see your results.

MarkMageeAstro commented 1 month ago

Nice work Sarthak, the plot seems to be coming along nicely.

I think we should have a consistent style across both matplotlib and plotly -- they should both look as close as possible to each other. I prefer the style of the current plotly version so I would try and update the matplotlib version to match it. Although I would also use a step plot for both rather than filled histograms. I prefer steps to filled because in the filled histograms you can get overlapping, translucent colours and that means they might not exactly match what's shown in the legend.

sarthak-dv commented 1 month ago

Thanks, @MarkMageeAstro for the feedback! I'll implement the changes to ensure that Matplotlib matches the current Plotly style, and I'll switch both to use step plots instead of filled histograms.

MarkMageeAstro commented 1 month ago

Rather than specifying bins as an integer in your hist function, you can give it a list of values. I'm not sure exactly what the right parameter name will be, but there will be some property like self.velocity or self.simulation_state.velocity, something like that. It should give you an array of the inner and outer boundaries of the cells in the model that you can pass to hist. This way you can be sure the plot lines up exactly with the cells in the model for all elements

sarthak-dv commented 1 month ago

@MarkMageeAstro @jamesgillanders I have generated step plots in both Plotly and Matplotlib, adjusted the bins based on the velocity array. Please review them and confirm if they meet your expectations.

Matplotlib:

matplotlib(step)

Plotly:

plotly(step)