Improve coordinate conversion speed #328

Varunvaruns9 · 2019-08-21T19:08:03Z

Combine core and velocity coordinates into one.
Make base conversion classes which operate only on the values without astropy.

pep8speaks · 2019-08-21T19:08:06Z

Hello @Varunvaruns9! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file src/einsteinpy/coordinates/utils.py:

Line 66:9: W503 line break before binary operator
Line 68:13: W503 line break before binary operator
Line 110:9: W503 line break before binary operator
Line 111:9: W503 line break before binary operator
Line 115:9: W503 line break before binary operator
Line 116:9: W503 line break before binary operator
Line 155:9: W503 line break before binary operator
Line 156:9: W503 line break before binary operator
Line 160:9: W503 line break before binary operator
Line 161:9: W503 line break before binary operator

Comment last updated at 2019-09-05 19:40:54 UTC

codecov · 2019-08-23T04:49:52Z

Codecov Report

Merging #328 into master will decrease coverage by 0.34%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #328      +/-   ##
==========================================
- Coverage   94.42%   94.08%   -0.35%     
==========================================
  Files          41       43       +2     
  Lines        1560     1673     +113     
==========================================
+ Hits         1473     1574     +101     
- Misses         87       99      +12

Impacted Files	Coverage Δ
...rc/einsteinpy/plotting/senile/geodesics_scatter.py	`94.33% <ø> (ø)`	⬆️
src/einsteinpy/coordinates/conversion.py	`100% <100%> (ø)`
src/einsteinpy/metric/kerrnewman.py	`91.66% <100%> (ø)`	⬆️
src/einsteinpy/coordinates/velocity.py	`100% <100%> (ø)`	⬆️
src/einsteinpy/coordinates/utils.py	`100% <100%> (ø)`
src/einsteinpy/metric/schwarzschild.py	`94.52% <100%> (ø)`	⬆️
src/einsteinpy/coordinates/core.py	`92.95% <100%> (-1.85%)`	⬇️
src/einsteinpy/coordinates/__init__.py	`100% <100%> (ø)`	⬆️
src/einsteinpy/metric/kerr.py	`91.02% <100%> (ø)`	⬆️
src/einsteinpy/symbolic/stress_energy_momentum.py	`95.83% <0%> (-4.17%)`	⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3bf6ac...79ec505. Read the comment docs.

Varunvaruns9 · 2019-08-23T09:44:18Z

Here is what I've been able to get by switching to conversions for converting coordinates:

For comparison, this is how much time it took before:

@shreyasbapat

ritzvik · 2019-08-23T11:24:42Z

Woah! That's a huge PR!

shreyasbapat

Cool. I will still make some changes, break up methods to use them with numba!

shreyasbapat · 2019-08-23T13:59:51Z

docs/source/api/coordinates/frame.rst

@@ -0,0 +1,7 @@
+frame module
+===============


Hmmmmmm, less number of '='s

shreyasbapat · 2019-08-23T14:00:49Z

docs/source/user_guide.rst

                                  145.45557 * u.km/u.s, 251.93643748389 * u.km/u.s, 0 * u.km/u.s)

-        bl_coord = pos_vel_coord.bl_differential(a)
+        bl_coord = pos_vel_coord.to_bl(a)


:/ .to(BoyerLindquist)

These are all API breaking changes!

@ritzvik Yes they are, should I change it to to(classname) or use the old methods?

@ritzvik Yes they are, should I change it to to(classname) or use the old methods?

to(classname) seems a good solution. You can add it but we do need to preserve the old methods also, atleast before 0.3.0

:/ .to(BoyerLindquist)

.to(BoyerLinduist, a) because a is definitely required to change other systems into BL. Maybe we can make the .to function like this?
to(coordinate_class, *args) or to(coordinate_class, **kwargs)

ritzvik · 2019-08-23T17:14:07Z

@Varunvaruns9 this is really a great PR. Wait a little bit, I will be reviewing and may be I can point out where the changes can be made, so that it does not break any APIs

Varunvaruns9 · 2019-08-24T03:38:23Z

@ritzvik @shreyasbapat Merged the changes.

ritzvik · 2019-08-24T03:42:33Z

@Varunvaruns9 Well, you can now implement the .to(CoordinateClass) method as suggested by @shreyasbapat

Also, tests are taking just one and a half minutes now as opposed to 10 mins earlier :)

Varunvaruns9 · 2019-08-24T04:04:45Z

I will start working on that after 29th, maybe we can merge this first and I'll make another PR for the new changes.

ritzvik · 2019-08-24T04:18:38Z

I will start working on that after 29th, maybe we can merge this first and I'll make another PR for the new changes.

That's okay! There's currently 25 commits in this PR, can you scale them down a bit, maybe 4-5 commits?

shreyasbapat · 2019-08-27T17:39:14Z

I messed up the mathematics somewhere. Atleast that's what I feel

ritzvik · 2019-08-28T17:09:10Z

src/einsteinpy/coordinates/utils.py

+
+
+def cartesian_to_spherical_fast(
+    x, y, z, v_x=None, v_y=None, v_z=None, velocities_provided=False


if this function is calling cartesian_to_spherical() & cartesian_to_spherical_novel(), shouldn't this function be defined below the other two. I know it doesn't matter in python but anyways, it's a commonly accepted practice in programming.

It is when we deal with compiled languages. ;D

it's a commonly accepted practice in programming

It's good to follow certain conventions, although!

We will fix it. Don't worry

ritzvik · 2019-08-28T17:10:52Z

src/einsteinpy/coordinates/utils.py

+    """
+    r = np.sqrt(x ** 2 + y ** 2 + z ** 2)
+    theta = np.arccos(z / r)
+    phi = np.arctan2(y, x)


you could have done r, theta, phi = cartesian_to_spherical_novel(x, y, z). Otherwise, there is code duplication.

I am not sure which versions of numba people might have.

ritzvik · 2019-08-28T17:15:53Z

src/einsteinpy/coordinates/utils.py

+):
+    if velocities_provided:
+        return cartesian_to_bl(x, y, z, a, v_x, v_y, v_z)
+    return cartesian_to_spherical_novel(x, y, z, a)


Silly mistake! cartesian_to_bl_novel(x, y, z, a)

ritzvik · 2019-08-28T17:33:33Z

@shreyasbapat I think you should revert the last two commits made by you as we have only 3 coordinate systems as of now, and adding computational graph would just complicate the code base. Let's aim it for 0.3.0.

Varunvaruns9 · 2019-09-02T08:36:25Z

@shreyasbapat Can we merge the first two commits? Then I can start working on adding new features to plotly plotter.

shreyasbapat · 2019-09-02T09:37:38Z

@ritzvik last two commits are not computational graph stuff :)

shreyasbapat · 2019-09-02T09:38:17Z

@Varunvaruns9 I guess yes. It's possible. Revert the commits

shreyasbapat · 2019-09-02T09:40:41Z

Or rather, give me 15 mins

Varunvaruns9 · 2019-09-03T13:02:12Z

@shreyasbapat Any progress?

shreyasbapat · 2019-09-04T21:25:36Z

Yeah, some minor patches

Varunvaruns9 · 2019-09-05T18:51:09Z

@shreyasbapat Fixed it. Theta and phi were switched 🤣

shreyasbapat · 2019-09-05T19:34:57Z

@shreyasbapat Fixed it. Theta and phi were switched rofl

Oh man! I was too frustrated!

shreyasbapat

Methods lack docstrings, but let's tackle that in another PR.

shreyasbapat · 2019-09-05T19:43:36Z

Manually Merged with 9ccf6d6

Varunvaruns9 force-pushed the faster branch from 6dd84e5 to fb3e867 Compare August 21, 2019 19:16

shreyasbapat self-requested a review August 22, 2019 08:19

Varunvaruns9 force-pushed the faster branch from b22031a to 30df329 Compare August 23, 2019 09:37

Varunvaruns9 changed the title ~~[WIP] Improve coordinate conversion speed~~ Improve coordinate conversion speed Aug 23, 2019

shreyasbapat approved these changes Aug 23, 2019

View reviewed changes

ritzvik approved these changes Aug 24, 2019

View reviewed changes

Varunvaruns9 force-pushed the faster branch from a0082b2 to beb74ea Compare August 24, 2019 05:06

ritzvik added the 2 - Ready label Aug 24, 2019

shreyasbapat removed the 2 - Ready label Aug 27, 2019

ritzvik requested changes Aug 28, 2019

View reviewed changes

shreyasbapat approved these changes Sep 5, 2019

View reviewed changes

Varunvaruns9 and others added 5 commits September 6, 2019 01:09

Add conversion module

95ea200

Use conversions in metric

1a9af04

njit all the functions in hope of getting faster outputs

1df7329

Fix some mathematical issues

fac4025

Fix conversion utils.

79ec505

shreyasbapat force-pushed the faster branch from b151db0 to 79ec505 Compare September 5, 2019 19:40

shreyasbapat merged commit 79ec505 into einsteinpy:master Sep 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve coordinate conversion speed #328

Improve coordinate conversion speed #328

Varunvaruns9 commented Aug 21, 2019

pep8speaks commented Aug 21, 2019 •

edited

codecov bot commented Aug 23, 2019 •

edited

Varunvaruns9 commented Aug 23, 2019

ritzvik commented Aug 23, 2019

shreyasbapat left a comment

shreyasbapat Aug 23, 2019

shreyasbapat Aug 23, 2019

ritzvik Aug 23, 2019

Varunvaruns9 Aug 23, 2019

ritzvik Aug 23, 2019

ritzvik commented Aug 23, 2019

Varunvaruns9 commented Aug 24, 2019

ritzvik commented Aug 24, 2019

Varunvaruns9 commented Aug 24, 2019

ritzvik commented Aug 24, 2019

shreyasbapat commented Aug 27, 2019

ritzvik Aug 28, 2019

shreyasbapat Sep 2, 2019

ritzvik Sep 2, 2019

shreyasbapat Sep 5, 2019

ritzvik Aug 28, 2019

shreyasbapat Sep 2, 2019

ritzvik Aug 28, 2019

ritzvik commented Aug 28, 2019

Varunvaruns9 commented Sep 2, 2019

shreyasbapat commented Sep 2, 2019

shreyasbapat commented Sep 2, 2019

shreyasbapat commented Sep 2, 2019

Varunvaruns9 commented Sep 3, 2019

shreyasbapat commented Sep 4, 2019

Varunvaruns9 commented Sep 5, 2019

shreyasbapat commented Sep 5, 2019

shreyasbapat left a comment

shreyasbapat commented Sep 5, 2019



		def cartesian_to_spherical_fast(
		x, y, z, v_x=None, v_y=None, v_z=None, velocities_provided=False

Improve coordinate conversion speed #328

Improve coordinate conversion speed #328

Conversation

Varunvaruns9 commented Aug 21, 2019

pep8speaks commented Aug 21, 2019 • edited

Comment last updated at 2019-09-05 19:40:54 UTC

codecov bot commented Aug 23, 2019 • edited

Codecov Report

Varunvaruns9 commented Aug 23, 2019

ritzvik commented Aug 23, 2019

shreyasbapat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritzvik commented Aug 23, 2019

Varunvaruns9 commented Aug 24, 2019

ritzvik commented Aug 24, 2019

Varunvaruns9 commented Aug 24, 2019

ritzvik commented Aug 24, 2019

shreyasbapat commented Aug 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritzvik commented Aug 28, 2019

Varunvaruns9 commented Sep 2, 2019

shreyasbapat commented Sep 2, 2019

shreyasbapat commented Sep 2, 2019

shreyasbapat commented Sep 2, 2019

Varunvaruns9 commented Sep 3, 2019

shreyasbapat commented Sep 4, 2019

Varunvaruns9 commented Sep 5, 2019

shreyasbapat commented Sep 5, 2019

shreyasbapat left a comment

Choose a reason for hiding this comment

shreyasbapat commented Sep 5, 2019

pep8speaks commented Aug 21, 2019 •

edited

codecov bot commented Aug 23, 2019 •

edited