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.

Sign up for GitHub

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

Jump to bottom

Streamline the coordinates module #650

Open
1 task
JeS24 opened this issue Oct 6, 2023 · 10 comments
Open
1 task

Streamline the coordinates module #650

JeS24 opened this issue Oct 6, 2023 · 10 comments
Labels
coordinates good first issue Good for newcomers hacktoberfest Hacktoberfest 2023
Milestone
0.5.0

Comments

@JeS24
Copy link
Member

JeS24 commented Oct 6, 2023 •
edited
Loading

🐞 Problem

Currently, there are many redundancies in einsteinpy.coordinates. There's a ton of code rewrite instead of reuse between the various modules. This was done for various reasons, but the primary reason was to enable numba.jit acceleration. Today, it should be possible to keep numba.jit functionality while reducing the size of the module codebase, making it more robust and maintainable. For instance, see #644. Some key things to note are as follows:

  1. Move away from named variables for each coordinate type (e.g., t x y z in Cartesian, t r theta phi in Polar, etc.). Use generic names such as e0, ..., e3 for position components and u0, ..., u3 for velocity components.
    • This will require changing names at various other places, like in the constructor in bodies.py. Also if a BaseDifferential / base class is added.
  2. Refactor coordinates/utils.py to separate out the position and velocity conversion parts and to reassemble them, based on whether the velocity components are provided. This ensures that individual conversion operations are still accelerated using jit.
  3. Investigate if it is possible to have a base coordinate class, that can contain most of the reusable code for each Coordinate system.

Note that this is a simple refactor of the module, which does not necessarily add any new functionality. However, it might pave the way for adding newer coordinate systems more easily.

💡 Possible solutions

See the discussion in #644.

📋 Contribution steps

Please consult the developer docs to set up your dev environment and get started. Also, feel free to communicate, if you hit a snag. You can do so in our Gitter or Matrix/Element chat rooms (recommended), or you can also reply here.

  • Comment below about what you've started working on.
  • Add, commit, and push your changes
  • Submit a pull request.
  • Add this in the comments:
    • Addresses #650 if you are partially fixing the issue.
    • Fixes #650 if you are completely fixing the issue.
  • Ask for a review in the comments section of the pull request
  • Celebrate your contribution to this project 🎉
@JeS24 JeS24 added good first issue Good for newcomers coordinates hacktoberfest Hacktoberfest 2023 labels Oct 6, 2023
@JeS24 JeS24 added this to the 0.5.0 milestone Oct 6, 2023
@GaganaMD
Copy link

GaganaMD commented Oct 7, 2023

Hey! I'd like to be assigned to this issue.

@JeS24
Copy link
Member Author

JeS24 commented Oct 7, 2023

@GaganaMD Sure. Please go ahead.

@JeS24 JeS24 mentioned this issue Oct 7, 2023
Open
5 tasks
@TONYOG12
Copy link

TONYOG12 commented Dec 7, 2023 •
edited
Loading

Do you need any assistance on this issue, I would be happy to help @GaganaMD

@dana-gill
Copy link

dana-gill commented Jan 25, 2024 •
edited
Loading

Hi there! I've decided to start working on

Refactor coordinates/utils.py to separate out the position and velocity conversion parts and to reassemble them, based on whether the velocity components are provided. This ensure that individual conversion operations are still accelerated using jit.

Edit: I just looked through the code and it looks like this was already done

@TONYOG12
Copy link

Oh okay. Then does this Issue need to be closed? or is this part of the issue still open?:

  1. Investigate if it is possible to have a base coordinate class, that can contain most of the reusable code for each Coordinate system.

@JeS24
Copy link
Member Author

JeS24 commented Jan 29, 2024

Hi there! I've decided to start working on

Refactor coordinates/utils.py to separate out the position and velocity conversion parts and to reassemble them, based on whether the velocity components are provided. This ensure that individual conversion operations are still accelerated using jit.

Edit: I just looked through the code and it looks like this was already done

Weirdly, I was not notified of this comment, even though I am subscribed to this thread. Anyway, this refactor is yet to be done. The idea is to remove (or move into separate functions) rewritten code. For example, check these sets of lines from two functions in utils.py, concerned with BL to Cartesian conversion:

einsteinpy/src/einsteinpy/coordinates/utils.py

Lines 162 to 166 in e56fd9e

xa = np.sqrt(r**2 alpha**2)
sin_norm = xa * np.sin(th)
x = sin_norm * np.cos(p)
y = sin_norm * np.sin(p)
z = r * np.cos(th)

einsteinpy/src/einsteinpy/coordinates/utils.py

Lines 189 to 193 in e56fd9e

xa = np.sqrt(r**2 alpha**2)
sin_norm = xa * np.sin(th)
x = sin_norm * np.cos(p)
y = sin_norm * np.sin(p)
z = r * np.cos(th)

You can find similar code duplication for other coordinate system pairs, too. The reason it is currently written this way is that we are branching based on velocities_provided's value in bl_to_cartesian_fast() (L150), among other functions:

einsteinpy/src/einsteinpy/coordinates/utils.py

Lines 147 to 152 in e56fd9e

def bl_to_cartesian_fast(
t, r, th, p, alpha, v_r=None, v_th=None, v_p=None, velocities_provided=False
):
if velocities_provided:
return bl_to_cartesian(t, r, th, p, alpha, v_r, v_th, v_p)
return bl_to_cartesian_novel(t, r, th, p, alpha)

@TONYOG12 @dana-gill The goal of this refactor is to remove code duplication and instead do something like #644 (comment) (With one correction: if not any(vel) is not a great test for None, since it will also return True for all 0s. Using a list comprehension is a better method.). I hope this clears up the confusion.

@JeS24
Copy link
Member Author

JeS24 commented Jan 29, 2024

Also, feel free to work on the issue. All subparts are open.

@TONYOG12
Copy link

thank you for the additional context @JeS24. I have already began work on point 1. I'll create a pr for that and take a look at point 2. with @dana-gill

TONYOG12 added a commit to TONYOG12/einsteinpy that referenced this issue Jan 30, 2024
@TONYOG12
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
@TONYOG12 TONYOG12 mentioned this issue Jan 30, 2024
Open
TONYOG12 added a commit to TONYOG12/einsteinpy that referenced this issue Jan 30, 2024
@TONYOG12
fix failed tests on quality ci
caeae12 
addresses einsteinpy#650
@TONYOG12
Copy link

TONYOG12 commented Feb 2, 2024

Made quite a large pr to address this issue, could u kindly review it @JeS24
#656

@JeS24
Copy link
Member Author

JeS24 commented Feb 2, 2024

Made quite a large pr to address this issue, could u kindly review it @JeS24 #656

Sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
coordinates good first issue Good for newcomers hacktoberfest Hacktoberfest 2023
Projects
None yet
Milestone
0.5.0
Development

No branches or pull requests
4 participants
@JeS24 @TONYOG12 @GaganaMD @dana-gill