Repository: einsteinpy/einsteinpy

Issue:
Make coordinate objects subscriptable
🐞 **Problem**

We can't do a[0] with the coordinates right now, which is very intuitive. Thanks to @shilpi1958 , we know what needs to be done!
🎯 **Goal**

<!--- Why is this change important to you? How would you use it? -->
<!--- How can it benefit other users? -->

💡 **Possible solutions**

<!--- Not obligatory, but suggest an idea for implementing addition or change -->

📋  **Steps to solve the problem**

 * 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 🎉


Discussion:
pep8speaks: Hello @Raahul-Singh! Thanks for opening this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:

* In the file [`src/einsteinpy/coordinates/core.py`](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py):

> [Line 33:32](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L33): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 33:44](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L33): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 33:56](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L33): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 33:73](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L33): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 40:1](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L40): [W293](https://duckduckgo.com/?q=pep8%20W293) blank line contains whitespace
> [Line 41:25](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L41): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ','
> [Line 43:1](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L43): [W293](https://duckduckgo.com/?q=pep8%20W293) blank line contains whitespace
> [Line 145:32](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L145): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 145:48](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L145): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 145:66](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L145): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 145:85](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L145): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 155:25](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L155): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ','
> [Line 157:1](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L157): [W293](https://duckduckgo.com/?q=pep8%20W293) blank line contains whitespace
> [Line 228:32](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L228): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 228:48](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L228): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 228:66](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L228): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 228:80](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L228): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 228:95](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L228): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 238:25](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L238): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ','
> [Line 239:38](https://github.com/einsteinpy/einsteinpy/blob/a05750e37e1829b18885155982d98e98b63e4251/src/einsteinpy/coordinates/core.py#L239): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace


shreyasbapat: Hey @Raahul-Singh you need to take care of code linting. 

You will have to run `tox -e reformat` to fix this :)
Raahul-Singh: > Hey @Raahul-Singh you need to take care of code linting.
> 
> You will have to run `tox -e reformat` to fix this :)

And with one command you have helped me solve most of my pep8 issues. Thanks for this!!
codecov[bot]: # [Codecov](https://codecov.io/gh/einsteinpy/einsteinpy/pull/387?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@b9e1764`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `79.41%`.

[![Impacted file tree graph](https://codecov.io/gh/einsteinpy/einsteinpy/pull/387/graphs/tree.svg?width=650&token=H3oDVYA11f&height=150&src=pr)](https://codecov.io/gh/einsteinpy/einsteinpy/pull/387?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master     #387   +/-   ##
=========================================
  Coverage          ?   94.26%           
=========================================
  Files             ?       54           
  Lines             ?     1866           
  Branches          ?        0           
=========================================
  Hits              ?     1759           
  Misses            ?      107           
  Partials          ?        0
```


| [Impacted Files](https://codecov.io/gh/einsteinpy/einsteinpy/pull/387?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/einsteinpy/plotting/\_\_init\_\_.py](https://codecov.io/gh/einsteinpy/einsteinpy/pull/387/diff?src=pr&el=tree#diff-c3JjL2VpbnN0ZWlucHkvcGxvdHRpbmcvX19pbml0X18ucHk=) | `100% <100%> (ø)` | |
| [src/einsteinpy/plotting/hypersurface/\_\_init\_\_.py](https://codecov.io/gh/einsteinpy/einsteinpy/pull/387/diff?src=pr&el=tree#diff-c3JjL2VpbnN0ZWlucHkvcGxvdHRpbmcvaHlwZXJzdXJmYWNlL19faW5pdF9fLnB5) | `100% <100%> (ø)` | |
| [.../einsteinpy/hypersurface/schwarzschildembedding.py](https://codecov.io/gh/einsteinpy/einsteinpy/pull/387/diff?src=pr&el=tree#diff-c3JjL2VpbnN0ZWlucHkvaHlwZXJzdXJmYWNlL3NjaHdhcnpzY2hpbGRlbWJlZGRpbmcucHk=) | `94.2% <42.85%> (ø)` | |
| [src/einsteinpy/coordinates/core.py](https://codecov.io/gh/einsteinpy/einsteinpy/pull/387/diff?src=pr&el=tree#diff-c3JjL2VpbnN0ZWlucHkvY29vcmRpbmF0ZXMvY29yZS5weQ==) | `90% <85.71%> (ø)` | |
| [src/einsteinpy/plotting/hypersurface/core.py](https://codecov.io/gh/einsteinpy/einsteinpy/pull/387/diff?src=pr&el=tree#diff-c3JjL2VpbnN0ZWlucHkvcGxvdHRpbmcvaHlwZXJzdXJmYWNlL2NvcmUucHk=) | `88.88% <88.88%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/einsteinpy/einsteinpy/pull/387?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/einsteinpy/einsteinpy/pull/387?src=pr&el=footer). Last update [b9e1764...7a4c611](https://codecov.io/gh/einsteinpy/einsteinpy/pull/387?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

shreyasbapat: Also, this would need a rebase
Raahul-Singh: > Can we also write unit tests for this, that would make the PR mergeable, along with the changes O proposed.
> Can we do `self._dimension` instead ?

Where should I put the test? Which file?
I'm a little confused as this is a simple `__getitem__` implementation.
shreyasbapat: Sure. You can put tests in this file: 

https://github.com/einsteinpy/einsteinpy/blob/master/src/einsteinpy/tests/test_coordinates/test_coord_transform.py
shreyasbapat: Still, we need to be sure that it works always! :) 
ritzvik: My two cents for this PR.
Can we make `obj[0]`, `obj[1]` and `obj[2]` the aliases for `obj["x"]`, `obj["y"]` and `obj["z"]` ? Same for `r, theta, phi`. 

Anyway, @Raahul-Singh , wait for @shreyasbapat go-ahead before implementing this.
shreyasbapat: My end goal with the issue was the same. But then it came to my mind, how do we know which one is 0,1 and 2. If you feel this is not a big issue @ritzvik , and have some solution. I am more inclined to the solution you suggested.
ritzvik: I guess it's pretty evident in Cartesian case, `0,1,2` for `x,y,z`.
`r, theta, phi` is the order used in [wikipedia](https://en.wikipedia.org/wiki/Spherical_coordinate_system), so we can go with that in case of Spherical and BL.

We can add these entries into the same `self.coordinates` as coded by @Raahul-Singh , where `0,1,2` can be the keys and `self.x, self.y, self.z` can be the values. What do you say @shreyasbapat ?

Also, @Raahul-Singh , please change the same to another other than `self.coordinates`. It seems very ambiguous
shreyasbapat: If you feel that's obvious, I am fine. 
Rename it to `self._dimensions`
shreyasbapat: This can't show up in the docs. We need to make it private
Raahul-Singh: Hey! I've changed `self.coordinates` to `self._dimension` . I've also added a test. The simplest way to make this subscript-able by index is to use a tuple rather than a dictionary. Tuple instead of a list because list is mutable. However, `self._dimension[-1]` would get the name of the coordinate whereas all others would get quantities. Would this not cause occasional errors, if the user is not familiar with it?
shreyasbapat: > Hey! I've changed `self.coordinates` to `self._dimension` . I've also added a test. The simplest way to make this subscript-able by index is to use a tuple rather than a dictionary. Tuple instead of a list because list is mutable. However, `self._dimension[-1]` would get the name of the coordinate whereas all others would get quantities. Would this not cause occasional errors, if the user is not familiar with it?

I guess we will document that somewhere. Put a comment in the code for the same. I will get it sorted in my next PR. And please fix the sggestions by @ritzvik . And rebase and squash!
shreyasbapat: @Raahul-Singh are you still on it?
Raahul-Singh: > @Raahul-Singh are you still on it?

Oh, yes. I'll get it done by tonight. Just a busy couple of days. 😅 I'm sorry. 
Raahul-Singh: Does this work?

ritzvik: > Does this work?

Yups! Seems good enough!
BTW, maybe, you rebased the branch wrong. The PR is showing "25 files changed".
Raahul-Singh: > > Does this work?
> 
> Yups! Seems good enough!
> BTW, maybe, you rebased the branch wrong. The PR is showing "25 files changed".

Sorry. :sweat_smile: I am not that experienced with rebasing yet. Could you help me with fixing it?
ritzvik: I guess you somehow included all the previous commits in 
21d9a97 .

So I would say, go to the project root and do this:
```
git reset --hard HEAD~3 # makes a hard reset to 3 commits behind
git checkout dfe2eac -- ./src/einsteinpy/coordinates/core.py # copies the file from your latest commit
```
You would find `core.py` file staged and make a new commit. After this, rebase with upstream
```
git pull --rebase upstream master
```

I believe the changes in test file are pretty small. Just copy-paste them and create another commit.

That's it

Patch:
diff --git a/src/einsteinpy/coordinates/core.py b/src/einsteinpy/coordinates/core.py
index a0611bc..16c8936 100644
--- a/src/einsteinpy/coordinates/core.py
+++ b/src/einsteinpy/coordinates/core.py
@@ -30,6 +30,13 @@ class Cartesian(CartesianConversion):
         self.z = z
         super().__init__(x.si.value, y.si.value, z.si.value)
         self.system = "Cartesian"
+        self._dimension = {
+            "x": self.x,
+            "y": self.y,
+            "z": self.z,
+            "system": "Cartesian",
+        }
+        self._dimension_order = ("x", "y", "z")
 
     def __repr__(self):
         return "Cartesian x: {}, y: {}, z: {}".format(self.x, self.y, self.z)
@@ -37,6 +44,22 @@ class Cartesian(CartesianConversion):
     def __str__(self):
         return self.__repr__()
 
+    def __getitem__(self, item):
+        """
+        Method to return coordinates. 
+        Objects are subsctiptable with both explicit names of parameters
+        and integer indices.
+
+        Parameters
+        ----------
+        item : ~astropy.units.quantity.Quantity or integer
+                Name of the parameter or it's index.
+                If `system`, Name of coordinate is returned.
+        """
+        if isinstance(item, (int, np.integer)):
+            return self._dimension[self._dimension_order[item]]
+        return self._dimension[item]
+
     def si_values(self):
         """
         Function for returning values in SI units.
@@ -138,6 +161,13 @@ class Spherical(SphericalConversion):
         self.phi = phi
         super().__init__(r.si.value, theta.si.value, phi.si.value)
         self.system = "Spherical"
+        self._dimension = {
+            "r": self.r,
+            "theta": self.theta,
+            "phi": self.phi,
+            "system": "Spherical",
+        }
+        self._dimension_order = ("r", "theta", "phi")
 
     def __repr__(self):
         return "Spherical r: {}, theta: {}, phi: {}".format(
@@ -147,6 +177,22 @@ class Spherical(SphericalConversion):
     def __str__(self):
         return self.__repr__()
 
+    def __getitem__(self, item):
+        """
+        Method to return coordinates. 
+        Objects are subsctiptable with both explicit names of parameters
+        and integer indices.
+
+        Parameters
+        ----------
+        item : ~astropy.units.quantity.Quantity or integer
+                Name of the parameter or it's index.
+                If `system`, Name of coordinate is returned.
+        """
+        if isinstance(item, (int, np.integer)):
+            return self._dimension[self._dimension_order[item]]
+        return self._dimension[item]
+
     def si_values(self):
         """
         Function for returning values in SI units.
@@ -217,6 +263,14 @@ class BoyerLindquist(BoyerLindquistConversion):
         self.a = a
         super().__init__(r.si.value, theta.si.value, phi.si.value, a=a.si.value)
         self.system = "BoyerLindquist"
+        self._dimension = {
+            "r": self.r,
+            "theta": self.theta,
+            "phi": self.phi,
+            "a": self.a,
+            "system": "BoyerLindquist",
+        }
+        self._dimension_order = ("r", "theta", "phi")
 
     def __repr__(self):
         return "Boyer-Lindquist r: {}, theta: {}, phi: {} | a: {}".format(
@@ -226,6 +280,22 @@ class BoyerLindquist(BoyerLindquistConversion):
     def __str__(self):
         return self.__repr__()
 
+    def __getitem__(self, item):
+        """
+        Method to return coordinates. 
+        Objects are subsctiptable with both explicit names of parameters
+        and integer indices.
+
+        Parameters
+        ----------
+        item : ~astropy.units.quantity.Quantity or integer
+                Name of the parameter or it's index.
+                If `system`, Name of coordinate is returned.
+        """
+        if isinstance(item, (int, np.integer)):
+            return self._dimension[self._dimension_order[item]]
+        return self._dimension[item]
+
     def si_values(self):
         """
         Function for returning values in SI units.


Test patch:
diff --git a/src/einsteinpy/tests/test_coordinates/test_coord_transform.py b/src/einsteinpy/tests/test_coordinates/test_coord_transform.py
index f8d88ae..303b1b4 100644
--- a/src/einsteinpy/tests/test_coordinates/test_coord_transform.py
+++ b/src/einsteinpy/tests/test_coordinates/test_coord_transform.py
@@ -154,3 +154,9 @@ def test_print_core_objects(cartesian, spherical, boyerlindquist):
 
     # again switch to old stdout
     sys.stdout = old_stdout
+
+
+def test_coordinate_subscripting(cartesian, spherical, boyerlindquist):
+    assert cartesian["x"] == cartesian.x == cartesian[0]
+    assert spherical["theta"] == spherical.theta == spherical[1]
+    assert boyerlindquist["phi"] == boyerlindquist.phi == boyerlindquist[2]