Use generic named variables in coordinates module #656

TONYOG12 · 2024-01-30T02:26:42Z

This commit addresses the first point on issue #650 by replacing named variables for each coordinate type with generic variable names

Addresses #650

@JeS24

This commit addresses the first point on issue einsteinpy#650 by replacing named variables for each coordinate type with generic variable names

addresses einsteinpy#650

pep8speaks · 2024-01-30T19:28:18Z

Hello @TONYOG12! 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 84:13: W503 line break before binary operator
Line 127:9: W503 line break before binary operator
Line 128:9: W503 line break before binary operator
Line 132:9: W503 line break before binary operator
Line 133:9: W503 line break before binary operator
Line 174:9: W503 line break before binary operator
Line 175:9: W503 line break before binary operator
Line 179:9: W503 line break before binary operator
Line 180:9: W503 line break before binary operator

In the file tests/test_metric/test_kerrnewman.py:

Line 218:72: W504 line break after binary operator

Comment last updated at 2024-06-18 12:27:02 UTC

codecov · 2024-01-30T19:35:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.13%. Comparing base (cffc0ca) to head (abfd6f6).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #656       /-   ##
==========================================
- Coverage   97.33%   97.13%   -0.21%     
==========================================
  Files          68       68              
  Lines        2515     2336     -179     
==========================================
- Hits         2448     2269     -179     
  Misses         67       67

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

addresses point 2 of einsteinpy#656

JeS24 · 2024-02-02T22:47:41Z

bin/geodesics_units/old_test_geodesics.py

This file is not tested. Is mypy giving issues?

src/einsteinpy/coordinates/conversion.py

src/einsteinpy/coordinates/core.py

src/einsteinpy/coordinates/differential.py

JeS24 · 2024-02-02T23:16:31Z

src/einsteinpy/coordinates/utils.py

-        return cartesian_to_spherical(t, x, y, z, v_x, v_y, v_z)
-    return cartesian_to_spherical_novel(t, x, y, z)
+@jit
+def calculate_coordinates(r, th, p, alpha):


Is the refactoring approach in #644 (comment) causing any issues ? (Apart from not any) ? That way should lead to clearer and more compact code. The idea is that the position and velocity conversions are jit-compiled, and these function names (cartesian_to_spherical vs cartesian_to_spherical_novel) are changed to something more comprehensible.

Alright thanks for the pointers, will refactor based on that approach

JeS24 · 2024-02-02T23:22:07Z

Notes for posterity:

@JeS24 Add "Breaking Change" to the merge commit.
These files only had modifications due to code formatting and style changes.

added the base classes to eliminate code repetition

JeS24

@TONYOG12 Thank you for the updated commit. I have reviewed the changes again. Besides these, the utils changes are left (function names change, see #656 (comment) & #644 (comment)).

JeS24 · 2024-04-30T01:18:12Z

src/einsteinpy/coordinates/core.py

            "system": self.system,
        }
-        self._dimension_order = ("t", "x", "y", "z")
+        self._dimension_order = ("e0", "e1", "e2", "e3")

    def __str__(self):


You can add the __str__ and __repr__ methods in the Base class itself. Instead of child-class name, like "Cartesian Coordinates", you can use f"{self.__class__.__name__} Coordinates \n ...".

Similarly, __getitem__ and position can also be part of the Base class.

As for member variables, self._dimension and self._dimension_order are the same across the 3 classes and can kept in the Base class.

JeS24 · 2024-04-30T01:25:23Z

src/einsteinpy/coordinates/conversion.py

            z-Component of 3-Position
-        v_x : float, optional
+        u0 : float, optional


u0 should correspond to the temporal component of velocity, like how e0 is the temporal component for (4-)position. So, here, you can start at u1, u2, u3 and rename v_t to u0 elsewhere (e.g., in differential.py)

JeS24 · 2024-04-30T01:42:14Z

src/einsteinpy/coordinates/differential.py

+        e0=u.s, e1=u.m, e2=u.m, e3=u.m, u0=u.m / u.s, u1=u.m / u.s, u2=u.m / u.s
+    )
+    def __init__(self, e0, e1, e2, e3, u0, u1, u2):
+        super().__init__(e0, e1, e2, e3, u0, u1, u2)
        self.system = "Cartesian"

    def __str__(self):


As with BaseCoordinate in core.py, the __str__ and __repr__ methods can be moved to the BaseDifferential class using the {self.__class__.__name__} trick.

Similarly, position and velocity methods can be in the Base class, as they do not change for the child-classes. The same also applies to the v_t (rename to u0) property and its setter. Both can be moved to the Base class.

JeS24 · 2024-04-30T02:14:40Z

src/einsteinpy/coordinates/utils.py

+def cartesian_to_bl_fast(e0, e1, e2, e3, e4, u0=None, u1=None, u2=None):
+    vel = [u0, u1, u2]
+    if all(u is not None for u in vel) and any(u != 0 for u in vel):
+        return cartesian_to_bl(e0, e1, e2, e3, e4, u0, u1, u2)


e4 is actually u0 (v_t). Refer to the comment in src/einsteinpy/coordinates/conversion.py.

…e classes

this commit addresses einsteinpy#650

93aaba7

This commit addresses the first point on issue einsteinpy#650 by replacing named variables for each coordinate type with generic variable names

auto-assign bot requested a review from JeS24 January 30, 2024 02:26

TONYOG12 added 2 commits January 30, 2024 02:31

fix failed tests on quality ci

caeae12

addresses einsteinpy#650

Update differential.py

7a99950

TONYOG12 changed the title ~~addresses point 1 of #650~~ Use generic named variables in coordinates module Jan 30, 2024

TONYOG12 marked this pull request as draft January 30, 2024 02:41

TONYOG12 added 2 commits January 30, 2024 17:23

fix failing tests

2647f31

fix test conversions.py errors

54c7414

TONYOG12 added 2 commits January 30, 2024 19:39

updates to adhere to pep8

233a8d2

address pep8 issues

21e776c

TONYOG12 marked this pull request as ready for review January 30, 2024 20:04

TONYOG12 added 4 commits January 31, 2024 15:28

remove _velocities_provided and code duplication

f3ef3f3

addresses point 2 of einsteinpy#656

Update utils.py

e6f0a7a

explicit typing for calculate_coordinates function

8a9906c

Update utils.py

01e5018

TONYOG12 mentioned this pull request Feb 2, 2024

Streamline the coordinates module #650

Open

1 task

JeS24 added coordinates 1 - In Progress labels Feb 2, 2024

JeS24 reviewed Feb 2, 2024

View reviewed changes

Breaking Change

a88d365

added the base classes to eliminate code repetition

JeS24 requested changes Apr 30, 2024

View reviewed changes

move __str__, __repr__, __getitem__, position and system into the bas…

164616f

…e classes

TONYOG12 marked this pull request as draft May 20, 2024 15:57

TONYOG12 added 3 commits May 20, 2024 16:19

Update differential.py

35efca6

Merge branch 'main' into streamline-the-coordinates-module

49ca159

update timelike component of 4-v

b6d8e58

TONYOG12 requested a review from JeS24 June 17, 2024 15:42

TONYOG12 marked this pull request as ready for review June 17, 2024 15:43

Update differential.py

abfd6f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use generic named variables in coordinates module #656

Use generic named variables in coordinates module #656

TONYOG12 commented Jan 30, 2024

pep8speaks commented Jan 30, 2024 •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading

JeS24 Feb 2, 2024

JeS24 Feb 2, 2024 •

edited

Loading

TONYOG12 Feb 10, 2024

JeS24 commented Feb 2, 2024

JeS24 left a comment •

edited

Loading

JeS24 Apr 30, 2024

JeS24 Apr 30, 2024

JeS24 Apr 30, 2024

TONYOG12 Jun 17, 2024

JeS24 Apr 30, 2024

TONYOG12 Jun 17, 2024

JeS24 Apr 30, 2024

JeS24 Apr 30, 2024

TONYOG12 Jun 17, 2024

JeS24 Apr 30, 2024

Use generic named variables in coordinates module #656

Are you sure you want to change the base?

Use generic named variables in coordinates module #656

Conversation

TONYOG12 commented Jan 30, 2024

pep8speaks commented Jan 30, 2024 • edited Loading

Comment last updated at 2024-06-18 12:27:02 UTC

codecov bot commented Jan 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

JeS24 Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeS24 commented Feb 2, 2024

JeS24 left a comment • edited Loading

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

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

pep8speaks commented Jan 30, 2024 •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading

JeS24 Feb 2, 2024 •

edited

Loading

JeS24 left a comment •

edited

Loading