Fixes the bug associated with axes_coordinates #189

yashrsharma44 · 2019-07-28T14:48:50Z

This PR addresses the bug associated with axes_coordinates.
This PR addresses the following problems -

Make sure the axes_coordinates for >2D plot contains no external values if axes_coordinate=None along plot_axis_indices. This makes sure that axes_coordinates is a list of None.
Edited _derive_axes_coordinates to calculate the values for independent and dependent axes. If dependent axes, then the values are downscaled to the data_shape along the axis by taking mean along non-plotting axes.
Added the support for getting edges in _derive_axes_coordinates .

Note that this PR should be merged after #176 is merged.

ghost · 2019-07-28T14:48:53Z

Thanks for the pull request @yashrsharma44!

I am a bot that checks pull requests for milestones and changelog entries. If you have any questions about what I am saying, please ask!
I have the following to report on this pull request:

This pull request does not have a milestone assigned to it. Only maintainers can change this, so you don't need to worry about it. 😄
I didn't detect a changelog file in this pull request. Please add a changelog file to the changelog/ directory following the instructions in the changelog README.

If there are any issues with this message, please report them here.

pep8speaks · 2019-07-28T14:48:54Z

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-08-07 20:03:30 UTC

1. Make sure the axes_coordinates for >2D plot contains no external values if axes_coordinate=0 along plot_axis_indices. This makes sure that axes_coordinates is a list of None. 2. Edited `_derive_axes_coordinates` to calculate the values for independent and dependent axes. If dependent axes, then the values are downscaled to the data_shape along the axis. 3. Added the support for getting `edges` in `_derive_axes_coordinates` .

ndcube/mixins/plotting.py

DanRyanIrish · 2019-08-01T17:22:52Z

ndcube/mixins/plotting.py

+                    axis_label_text = self.world_axis_physical_types[i]
+
+                    index = utils.wcs.get_dependent_data_axes(self.wcs, i, self.missing_axes)
+                    reduce_axis = np.where(index == np.array(i))[0]


I don't think it's necessary to wrap i into a numpy array...?

Sure, it was more of a defensive approach, would change it!

Yeah, now I remember, since index is a numpy array, so for comparing the same objects, I have to convert i into a numpy array.

yashrsharma44 · 2019-08-02T15:13:25Z

Work to be done -

~~Documentation to use the new plotting paradigm.~~ To be covered in another PR.
Fix labelling of >2D NDCubes, i.e. fix the label issue of ImageAnimator/ImageAnimatorWCS.
Nitpickings?
Add docstrings entries.

Also Ping @DanRyanIrish for the changes.

ndcube/mixins/plotting.py

…ame dimension as of self.data

DanRyanIrish

In addition to the inline comments, the axis labelling commented out starting line here and here need to be fixed. Either by uncommenting, or rewriting depending on whether they are still breaking tests and add the labels to the axes.

ndcube/mixins/plotting.py

…d added the support for axis labels in 3D plotting

yashrsharma44 · 2019-08-05T15:14:13Z

Regarding the docstrings, I was thinking of adding a line explaining the type of axes_coordinates we want along with all the places where axes_coordinates are found.
#The physical coordinates expected by axes_coordinates should be an array of pixel_edges instead of pixel_centers.
@DanRyanIrish do you have any suggestions?

DanRyanIrish · 2019-08-05T15:16:47Z

No need to say instead of pixel centers. But don't forget that at the highest levels a str denoted extra coords should also be supported.

yashrsharma44 · 2019-08-05T15:19:19Z

But don't forget that at the highest levels a str denoted extra coords should also be supported.

Currently, we don't support getting edges for extra_coords.

DanRyanIrish · 2019-08-06T13:39:44Z

Don't we? Isn't that the role of _get_extra_coord_edges?

yashrsharma44 · 2019-08-07T11:14:41Z

Okay, I guess I didn't understand that. Yeah, for plotting we support extra_coords such that extra_coords can store pixel_centers and the plotting code internally converts it into pixel_edges. But currently, extra_coords has no parameter to get pixel_edges as of now.

DanRyanIrish · 2019-08-07T14:31:44Z

That's fine. Since the API is only for the user to enter the name of the extra_coord they want to use for the axis, the conversion to pixel edges can be hidden from the user.

DanRyanIrish · 2019-08-07T14:33:09Z

So add the fact that users can supply a string in axes_coordinates so long as it's a valid extra_coord name and that coord corresponds to the axis to which is it being assigned in the plot.

yashrsharma44 · 2019-08-07T17:50:17Z

I was thinking of adding this as a docstring for extra_coords -

# The physical coordinates expected by axes_coordinates can be string as well, which corresponds to a valid `extra_coords` and that coord corresponds to the axis to which is it being assigned in the plot.

yashrsharma44 · 2019-08-07T17:53:58Z

I guess the astropydev is failing due to the fact that it depends on SunPy stable release?

DanRyanIrish · 2019-08-07T18:32:45Z

I was thinking of adding this as a docstring for extra_coords -

# The physical coordinates expected by axes_coordinates can be string as well, which corresponds to a valid `extra_coords` and that coord corresponds to the axis to which is it being assigned in the plot.

How about:

A str entry in axes_coordinates signifies that an extra_coord will be used for the axis's coordinates.  The str must be a valid name of an extra_coord that corresponds to the same axis to which it is applied in the plot.

Does that make sense?

DanRyanIrish · 2019-08-07T18:33:02Z

I guess the astropydev is failing due to the fact that it depends on SunPy stable release?

Yes, only the sunpy_dev test has to pass.

DanRyanIrish · 2019-08-07T18:35:21Z

And yes, once the str info has been added to the docstrings we can merge :)

yashrsharma44 · 2019-08-07T18:41:49Z

And yes, once the str info has been added to the docstrings we can merge :)

Thanks @DanRyanIrish

DanRyanIrish

I noticed that this PR does not seem to handle extra_coord str entries to axes_coordinates correctly. Instead of holding up this PR, can you create an issue saying extra_coord axes_coordinates support must be extended to NDCubeSequence?

DanRyanIrish · 2019-08-07T18:46:01Z

ndcube/mixins/sequence_plotting.py

@@ -691,6 +697,8 @@ class ImageAnimatorNDCubeSequence(ImageAnimatorWCS):
        for that axis.
        The physical coordinates expected by axis_ranges should be an array of
        pixel_edges.
+        A str entry in axis_ranges signifies that an extra_coord will be used for the axis's coordinates.
+        The str must be a valid name of an extra_coord that corresponds to the same axis to which it is applied in the plot.


The str API is not valid for animator objects. Only for the NDCube and NDCubeSequence objects.

DanRyanIrish · 2019-08-07T18:46:58Z

ndcube/mixins/sequence_plotting.py

@@ -788,6 +796,8 @@ class ImageAnimatorCubeLikeNDCubeSequence(ImageAnimatorWCS):
        for that axis.
        The physical coordinates expected by axis_ranges should be an array of
        pixel_edges.
+        A str entry in axis_ranges signifies that an extra_coord will be used for the axis's coordinates.
+        The str must be a valid name of an extra_coord that corresponds to the same axis to which it is applied in the plot.


Same as above. The str API is not valid for animator objects. The extra coord edges must be derived before entering them into the animator objects.

DanRyanIrish · 2019-08-07T18:47:09Z

ndcube/mixins/sequence_plotting.py

@@ -907,6 +917,8 @@ class LineAnimatorNDCubeSequence(LineAnimator):
        for that axis.
        The physical coordinates expected by axis_ranges should be an array of
        pixel_edges.
+        A str entry in axis_ranges signifies that an extra_coord will be used for the axis's coordinates.
+        The str must be a valid name of an extra_coord that corresponds to the same axis to which it is applied in the plot.


Same as above.

DanRyanIrish · 2019-08-07T18:47:17Z

ndcube/mixins/sequence_plotting.py

@@ -1143,6 +1155,8 @@ class LineAnimatorCubeLikeNDCubeSequence(LineAnimator):
        for that axis.
        The physical coordinates expected by axis_ranges should be an array of
        pixel_edges.
+        A str entry in axis_ranges signifies that an extra_coord will be used for the axis's coordinates.
+        The str must be a valid name of an extra_coord that corresponds to the same axis to which it is applied in the plot.


Same as above

yashrsharma44 · 2019-08-07T20:06:13Z

I guess I am not clear with the comment above. Are you saying that extra_coords are not supported yet in sequence_plotting? It would be good if you could elaborate on this more.

DanRyanIrish

I think my comment may have just muddied the waters. I will withdraw it and if we as users discover any future issues with sequence plotting, they can be handled in a new PR. I think it shouldn't hold up this PR.

yashrsharma44 force-pushed the axes_coord_bug branch from 62293ed to 6e318ab Compare August 1, 2019 14:03

yashrsharma44 added 2 commits August 1, 2019 19:34

PEP8 fixes

81f8c04

yashrsharma44 force-pushed the axes_coord_bug branch from 6e318ab to 81f8c04 Compare August 1, 2019 14:05

DanRyanIrish reviewed Aug 1, 2019

View reviewed changes

Removed the rewriting of None values along the slider_axes

1360ae0

yashrsharma44 commented Aug 2, 2019

View reviewed changes

ndcube/mixins/plotting.py Outdated Show resolved Hide resolved

yashrsharma44 commented Aug 2, 2019

View reviewed changes

ndcube/mixins/plotting.py Outdated Show resolved Hide resolved

Added a check for reduction of data into 1D if data is not 1D or of s…

3ef2284

…ame dimension as of self.data

DanRyanIrish reviewed Aug 5, 2019

View reviewed changes

ndcube/mixins/plotting.py Outdated Show resolved Hide resolved

ndcube/mixins/plotting.py Outdated Show resolved Hide resolved

yashrsharma44 added 4 commits August 5, 2019 19:18

Changed xdata with new_axes_coordinate

7b7a5ae

Remove redundant numpy array conversion

7ea20e3

Modified the explanation for downscaling the dimension of the data an…

3d6f250

…d added the support for axis labels in 3D plotting

Added docstrings for the changes

b3cdea6

nitpickings for docstrings

bfbb391

Reinitiate CI

eedb321

yashrsharma44 added 2 commits August 8, 2019 00:08

Added docstring for str values in axes_coordinates

1b1a79c

PEP8 fixes

9daf4ed

DanRyanIrish reviewed Aug 7, 2019

View reviewed changes

Removed incorrect docstrings from sequence_plotting.py

da6c451

DanRyanIrish approved these changes Aug 8, 2019

View reviewed changes

DanRyanIrish merged commit a6ff775 into sunpy:master Aug 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes the bug associated with axes_coordinates #189

Fixes the bug associated with axes_coordinates #189

yashrsharma44 commented Jul 28, 2019 •

edited

Loading

ghost commented Jul 28, 2019 •

edited by ghost

Loading

pep8speaks commented Jul 28, 2019 •

edited

Loading

DanRyanIrish Aug 1, 2019

yashrsharma44 Aug 2, 2019

yashrsharma44 Aug 5, 2019

yashrsharma44 commented Aug 2, 2019 •

edited

Loading

DanRyanIrish left a comment

yashrsharma44 commented Aug 5, 2019

DanRyanIrish commented Aug 5, 2019

yashrsharma44 commented Aug 5, 2019

DanRyanIrish commented Aug 6, 2019

yashrsharma44 commented Aug 7, 2019

DanRyanIrish commented Aug 7, 2019

DanRyanIrish commented Aug 7, 2019

yashrsharma44 commented Aug 7, 2019

yashrsharma44 commented Aug 7, 2019 •

edited

Loading

DanRyanIrish commented Aug 7, 2019

DanRyanIrish commented Aug 7, 2019

DanRyanIrish commented Aug 7, 2019

yashrsharma44 commented Aug 7, 2019

DanRyanIrish left a comment

DanRyanIrish Aug 7, 2019

DanRyanIrish Aug 7, 2019

DanRyanIrish Aug 7, 2019

DanRyanIrish Aug 7, 2019

yashrsharma44 commented Aug 7, 2019

DanRyanIrish left a comment

Fixes the bug associated with axes_coordinates #189

Fixes the bug associated with axes_coordinates #189

Conversation

yashrsharma44 commented Jul 28, 2019 • edited Loading

ghost commented Jul 28, 2019 • edited by ghost Loading

pep8speaks commented Jul 28, 2019 • edited Loading

Comment last updated at 2019-08-07 20:03:30 UTC

DanRyanIrish Aug 1, 2019

Choose a reason for hiding this comment

yashrsharma44 Aug 2, 2019

Choose a reason for hiding this comment

yashrsharma44 Aug 5, 2019

Choose a reason for hiding this comment

yashrsharma44 commented Aug 2, 2019 • edited Loading

DanRyanIrish left a comment

Choose a reason for hiding this comment

yashrsharma44 commented Aug 5, 2019

DanRyanIrish commented Aug 5, 2019

yashrsharma44 commented Aug 5, 2019

DanRyanIrish commented Aug 6, 2019

yashrsharma44 commented Aug 7, 2019

DanRyanIrish commented Aug 7, 2019

DanRyanIrish commented Aug 7, 2019

yashrsharma44 commented Aug 7, 2019

yashrsharma44 commented Aug 7, 2019 • edited Loading

DanRyanIrish commented Aug 7, 2019

DanRyanIrish commented Aug 7, 2019

DanRyanIrish commented Aug 7, 2019

yashrsharma44 commented Aug 7, 2019

DanRyanIrish left a comment

Choose a reason for hiding this comment

DanRyanIrish Aug 7, 2019

Choose a reason for hiding this comment

DanRyanIrish Aug 7, 2019

Choose a reason for hiding this comment

DanRyanIrish Aug 7, 2019

Choose a reason for hiding this comment

DanRyanIrish Aug 7, 2019

Choose a reason for hiding this comment

yashrsharma44 commented Aug 7, 2019

DanRyanIrish left a comment

Choose a reason for hiding this comment

yashrsharma44 commented Jul 28, 2019 •

edited

Loading

ghost commented Jul 28, 2019 •

edited by ghost

Loading

pep8speaks commented Jul 28, 2019 •

edited

Loading

yashrsharma44 commented Aug 2, 2019 •

edited

Loading

yashrsharma44 commented Aug 7, 2019 •

edited

Loading