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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Geodesic Plotting improvements #624

Open
3 tasks
JeS24 opened this issue Nov 4, 2022 · 6 comments
Open
3 tasks

Geodesic Plotting improvements #624

JeS24 opened this issue Nov 4, 2022 · 6 comments
Labels
good first issue Good for newcomers hacktoberfest Hacktoberfest 2023 plotting
Milestone

Comments

@JeS24
Copy link
Member

JeS24 commented Nov 4, 2022

馃悶 What bugs you?

(Continued from #610)

#612 added an aspect ratio option to *GeodesicPlotters that helps avoid skewed plots. However, at the moment, we are using plotly's nomenclature for aspect modes for the InteractiveGeodesicPlotter and matplotlib's nomenclature for the StaticGeodesicPlotter. This is mainly because matplotlib is yet to finalize the API for the aspect options (targeted for 3.7.0 and also see this).

To me, matplotlib's names for the options make more sense than Plotly's. I think, it will be great to offer a unified set of aspect options, since the most used API arguments for both the plotting modules (InteractiveGeodesicPlotter and StaticGeodesicPlotter) follow the same naming convention. Of course, users can still make changes using the fig or ax attributes for both the plotters, if they want to.

We can also take a look into arguments such as figsize, that are also not uniform across these modules.

馃幆 Goal

To bring uniformity to the plotting API.

馃挕 Possible solutions

Once matplotlib finalizes the API around aspect and scaling, we can decide on a common set of names and offer them as options for aspect across both the modules. matplotlib's equal and plotly's data are going to the most relevant ones, but others can be useful too. So, the tasks are:

  • Decide on a set of options.
  • Add them to both plotting modules.
  • Also look into homogenizing arguments such as figsize.

馃搵 Steps to solve the problem

Refer: https://docs.einsteinpy.org/en/latest/dev_guide.html

  • Comment below about what you've started working on.
  • Add, commit, push your changes
  • Submit a pull request and add this in comments - Addresses #<put issue number here>
  • Ask for a review in comments section of pull request
  • Celebrate your contribution to this project 馃帀
@JeS24 JeS24 added good first issue Good for newcomers plotting labels Nov 4, 2022
@JeS24 JeS24 added this to the 0.5.0 milestone Nov 4, 2022
@mshumayl
Copy link
Contributor

mshumayl commented Nov 8, 2022

Hi @JeS24, I'd like to work on this once matplotlib v3.7 gets released.

@JeS24
Copy link
Member Author

JeS24 commented Nov 8, 2022

@mshumayl Sure.

@mshumayl
Copy link
Contributor

mshumayl commented Jan 26, 2023

Matplotlib just added the first release candidate for 3.7.0. By default, Matplotlib will now modify the aspect by changing the box aspect instead of the data limits (old behaviour is still accessible by setting adjustable='datalim').

If we were to follow Matplotlib's nomenclature, if I understand this right, the equivalent mapping for the aspect parameters between Matplotlib and Plotly are as follows:

Matplotlib API (self.ax.set_aspect()) Plotly API (self.fig.update_layout())
set_aspect('auto') scene_aspectmode='data'
set_aspect('equal') scene_aspectmode='manual' scene_aspectratio=dict(x=1, y=1, z=1)
set_aspect('equalxy') scene_aspectmode='manual' scene_aspectratio=dict(x=1, y=1)
set_aspect('equalxz') scene_aspectmode='manual' scene_aspectratio=dict(x=1, z=1)
set_aspect('equalyz') scene_aspectmode='manual' scene_aspectratio=dict(y=1, z=1)

@JeS24
Copy link
Member Author

JeS24 commented Jan 31, 2023

@mshumayl This looks good. Once 3.7.0 is out, I think, we can move forward with incorporating Matplotlib's naming scheme across static and interactive plotters.

@JeS24
Copy link
Member Author

JeS24 commented Jul 7, 2023

@mshumayl matplotlib 3.7.x is out. If you are available to contribute the prior discussed code, it would be deeply appreciated! I have a bit of free time at least for a couple of weeks. I could review and merge it quickly.

@mshumayl
Copy link
Contributor

mshumayl commented Jul 7, 2023

@JeS24 thank you for letting me know! Sure, I'll send in a PR soon. Thanks again for your time.

@JeS24 JeS24 added the hacktoberfest Hacktoberfest 2023 label Sep 24, 2023
@JeS24 JeS24 mentioned this issue Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers hacktoberfest Hacktoberfest 2023 plotting
Projects
None yet
Development

No branches or pull requests

2 participants