Support plotting to use pixel_edges instead of pixel_values #176

yashrsharma44 · 2019-05-25T13:00:54Z

This PR addresses the fix of using pixel_edges instead of pixel_values.
Partial fix #172
TODO:

Tests
Documentation
Using it for >1D

Ping @DanRyanIrish

ghost · 2019-05-25T13:00:59Z

Thanks for the pull request @yashrsharma44! Everything looks great!

yashrsharma44 · 2019-05-25T13:02:42Z

Note that this PR will pass all the tests for SunPy >= 1.0, so some tests have been modified to be run in SunPy >= 1.0.

ndcube/mixins/plotting.py

codecov · 2019-05-28T13:48:44Z

Codecov Report

Merging #176 into master will decrease coverage by 0.01%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
- Coverage   94.65%   94.63%   -0.02%     
==========================================
  Files          19       19              
  Lines        2264     2275      +11     
==========================================
+ Hits         2143     2153      +10     
- Misses        121      122       +1

Impacted Files	Coverage Δ
ndcube/tests/test_plotting.py	`97.97% <ø> (ø)`	⬆️
ndcube/mixins/plotting.py	`86.04% <100%> (+0.05%)`	⬆️
ndcube/utils/cube.py	`96.38% <90%> (-0.88%)`	⬇️

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 e97e3e0...1759825. Read the comment docs.

pep8speaks · 2019-05-29T18:31:53Z

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-07-31 15:41:29 UTC

yashrsharma44 · 2019-05-29T18:34:50Z

Tasks left if all the tests pass -

Add tests for the edited helper method
Nitpickings

DanRyanIrish · 2019-06-04T08:38:31Z

Hi @yashrsharma44. What's the status of this PR?

yashrsharma44 · 2019-06-05T17:48:07Z

The idea of working of plotting in >1D needs to be discussed. Plotting tests for 1D works well.

DanRyanIrish · 2019-06-11T16:36:08Z

OK, what are the issues in >1D?

ndcube/mixins/sequence_plotting.py

yashrsharma44 · 2019-06-12T22:11:19Z

ndcube/utils/cube.py

@@ -195,3 +196,59 @@ def _get_dimension_for_pixel(axis_length, edges):
        False stands for pixel_value, while True stands for pixel_edge
    """
    return axis_length+1 if edges else axis_length
+
+def _get_extra_coord_edges(value, axis=-1):


Maybe the name of the function can be changed to some appropriate one :P

If you are looking for suggested name change how about _infer_linear_extra_coord_edges?

ndcube/tests/test_sequence_plotting.py

yashrsharma44 · 2019-06-12T22:16:51Z

Tasks to follow -

Write documentation for accepting pixel_egdes instead of pixel_values.
Writing tests for get_extra_coord_edges
Write a test for the un-tested line.
Nitpickings

ndcube/tests/test_sequence_plotting.py

1. Now if the `axes_coordinates` is None, or a `str`, it converts the `pixel_values` generated into `pixel_edges`. 2. User defined `axes_coordinates` are not touched, as now we need to document somewhere that input should be `pixel_edges` not `pixel_values`.

…aaray`. 1. Now the `pixel_edges` can be calculated for a given `ndarray` along a given axis.

…nges of plotting for SunPy.

…ata_shape arrays

ndcube/mixins/sequence_plotting.py

yashrsharma44 · 2019-07-25T18:16:31Z

So currently, plotting with argument plot_axis_indices is working, and plotting with argument axes_coordinates is failing, so it needs to be discussed if the bug fix for axes_coordinates needs to be in this PR.

ndcube/mixins/plotting.py

DanRyanIrish · 2019-07-26T13:37:41Z

That's great progress @yashrsharma44. When you say axes_coordinates are failing do you mean specifically when using a string to use an extra coordinate, or do you mean more generally?

yashrsharma44 · 2019-07-26T14:09:40Z

When you say axes_coordinates are failing do you mean specifically when using a string to use an extra coordinate, or do you mean more generally?

When the axes_coordinates takes in a str value, then it fails, else it works well with manual array values.

DanRyanIrish · 2019-07-26T14:21:42Z

OK. Let's make dealing with axes_coordinates another PR.

So what is left to do in this PR?

yashrsharma44 · 2019-07-26T16:37:08Z

Documentation about the plotting, everything else should be working fine after #PR 3283 is merged.

DanRyanIrish · 2019-07-30T15:19:54Z

ndcube/tests/test_plotting.py

@@ -1,102 +1,107 @@
-# -*- coding: utf-8 -*-
+# -*- coding: utf-8 -*-# -*- coding: utf-8 -*-


What does this do?

My bad, it got uncommented twice.

DanRyanIrish · 2019-07-30T15:22:34Z

There are a lot of tests commented out. Should they not be uncommented before merging and fixed if necessary. If this is not desired can you explain why?

yashrsharma44 · 2019-07-30T17:10:44Z

Hey @DanRyanIrish , the tests that you observed test_plotting.py which had lot of test cases commented, was due to my confusion, in which, I stashed(git stash) changes from another branch, which got reflected in this branch. I did not see the changes, so I have pushed with the current changes.

DanRyanIrish · 2019-07-30T19:10:06Z

ndcube/utils/cube.py

+
+     # Checks for corner cases
+    if value is None:
+        return value


This comment still isn't address. Just delete the above two lines for code.

DanRyanIrish · 2019-07-30T19:13:06Z

ndcube/utils/cube.py

@@ -195,3 +196,59 @@ def _get_dimension_for_pixel(axis_length, edges):
        False stands for pixel_value, while True stands for pixel_edge
    """
    return axis_length+1 if edges else axis_length
+
+def _get_extra_coord_edges(value, axis=-1):


If you are looking for suggested name change how about _infer_linear_extra_coord_edges?

DanRyanIrish · 2019-07-30T19:20:42Z

Apart from the above reminder to delete a couple lines, I am happy with this so long as it passes the sunpy dev tests. It's up to you whether you want to actually change the name of _get_extra_coord_edges or not.

Although passing the dev tests will require sunpy PR #3283 to be merged, right?

yashrsharma44 · 2019-07-30T19:21:33Z

I guess the change of name of _get_extra_coord_edges can be dealt in a separate PR, as lot of changes have been stacked up :P.

yashrsharma44 · 2019-07-30T19:22:15Z

Apart from the above reminder to delete a couple lines, I am happy with this so long as it passes the sunpy dev tests. It's up to you whether you want to actually change the name of _get_extra_coord_edges or not.

Although passing the dev tests will require sunpy PR #3283 to be merged, right?

Yeah, it depends on PR #3283, so this is blocked on that.

DanRyanIrish · 2019-07-30T19:24:55Z

Apart from the above reminder to delete a couple lines, I am happy with this so long as it passes the sunpy dev tests. It's up to you whether you want to actually change the name of _get_extra_coord_edges or not.
Although passing the dev tests will require sunpy PR #3283 to be merged, right?

Yeah, it depends on PR #3283, so this is blocked on that.

OK. Let's get that merged then. I think it will be ready once you address those few outstanding comments.

DanRyanIrish · 2019-07-30T21:12:22Z

Approved! Pending sunpy dev tests passing after the merging of sunpy PR sunpy/sunpy#3283

DanRyanIrish · 2019-07-31T21:24:23Z

Well done @yashrsharma44!!

yashrsharma44 commented May 25, 2019

View reviewed changes

ndcube/mixins/plotting.py Show resolved Hide resolved

DanRyanIrish added this to the 1.2 milestone Jun 3, 2019

DanRyanIrish reviewed Jun 11, 2019

View reviewed changes

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

yashrsharma44 force-pushed the extra_coords_plot branch from 0760c42 to 8f0da6e Compare June 12, 2019 22:06

yashrsharma44 commented Jun 12, 2019

View reviewed changes

ndcube/tests/test_sequence_plotting.py Outdated Show resolved Hide resolved

yashrsharma44 added 10 commits July 8, 2019 19:35

Added edges parameter for getting pixel_edges instead of pixel_values

b072595

Corrected some changes of the plotting test

b124e6e

Added a helper function to get pixel_edges from pixel_values

09a2621

Added changes for supporting new API for sunpy plotting

65ccdb5

PEP8 fixes

2de3013

Removed stray comments

b4103db

Changed the helper function _get_extra_coord_edges to work with `nd…

f7982e2

…aaray`. 1. Now the `pixel_edges` can be calculated for a given `ndarray` along a given axis.

Modified the test_sequence_plotting.py to work with the new API cha…

d7638e1

…nges of plotting for SunPy.

Added the test which was creating the issue

3639eb0

yashrsharma44 force-pushed the extra_coords_plot branch from d6a39fd to 3639eb0 Compare July 9, 2019 14:25

yashrsharma44 added 4 commits July 10, 2019 01:18

Removed a portion of code which was using 1D axis_ranges instead of d…

7f256a5

…ata_shape arrays

Added some test changes

9948a5a

Changed the expected values to a more appropriate one

0642569

PEP8 fixes

4c197d7

DanRyanIrish reviewed Jul 16, 2019

View reviewed changes

ndcube/mixins/sequence_plotting.py Show resolved Hide resolved

yashrsharma44 mentioned this pull request Jul 22, 2019

Support 2D plotting in NDCube using APE14 #188

Merged

3 tasks

Corrected the shape of axes_ranges for 1D_animate_cube

90dc63c

DanRyanIrish reviewed Jul 26, 2019

View reviewed changes

ndcube/mixins/plotting.py Show resolved Hide resolved

yashrsharma44 added 3 commits July 26, 2019 20:35

Edited the comments for reducing the dimensions for plot_axis_indices

3e09f6a

Uncommented some parts of seq_plotting

fe975c4

PEP8 fixes

21eb3ea

yashrsharma44 mentioned this pull request Jul 28, 2019

Fixes the bug associated with axes_coordinates #189

Merged

DanRyanIrish reviewed Jul 30, 2019

View reviewed changes

yashrsharma44 force-pushed the extra_coords_plot branch from b61aac8 to 21eb3ea Compare July 30, 2019 17:08

DanRyanIrish requested changes Jul 30, 2019

View reviewed changes

Removed redundant part from _get_extra_coord_edges

c5197df

DanRyanIrish approved these changes Jul 30, 2019

View reviewed changes

yashrsharma44 added 3 commits July 31, 2019 20:49

Corrected the name axes_coordinates

c1416fe

Typo Error

ff51ffd

Minor changes in test cases

644fb72

DanRyanIrish merged commit 68083f2 into sunpy:master Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support plotting to use pixel_edges instead of pixel_values #176

Support plotting to use pixel_edges instead of pixel_values #176

yashrsharma44 commented May 25, 2019 •

edited

Loading

ghost commented May 25, 2019 •

edited by ghost

Loading

yashrsharma44 commented May 25, 2019

codecov bot commented May 28, 2019 •

edited

Loading

pep8speaks commented May 29, 2019 •

edited

Loading

yashrsharma44 commented May 29, 2019 •

edited

Loading

DanRyanIrish commented Jun 4, 2019

yashrsharma44 commented Jun 5, 2019

DanRyanIrish commented Jun 11, 2019

yashrsharma44 Jun 12, 2019

DanRyanIrish Jul 30, 2019

yashrsharma44 commented Jun 12, 2019 •

edited

Loading

yashrsharma44 commented Jul 25, 2019

DanRyanIrish commented Jul 26, 2019

yashrsharma44 commented Jul 26, 2019

DanRyanIrish commented Jul 26, 2019

yashrsharma44 commented Jul 26, 2019

DanRyanIrish Jul 30, 2019

yashrsharma44 Jul 30, 2019

DanRyanIrish commented Jul 30, 2019

yashrsharma44 commented Jul 30, 2019 •

edited

Loading

DanRyanIrish Jul 30, 2019

DanRyanIrish Jul 30, 2019

DanRyanIrish commented Jul 30, 2019

yashrsharma44 commented Jul 30, 2019

yashrsharma44 commented Jul 30, 2019

DanRyanIrish commented Jul 30, 2019

DanRyanIrish commented Jul 30, 2019 •

edited by Cadair

Loading

DanRyanIrish commented Jul 31, 2019

		@@ -1,102 +1,107 @@
		# -- coding: utf-8 --
		# -- coding: utf-8 --# -- coding: utf-8 --

Support plotting to use pixel_edges instead of pixel_values #176

Support plotting to use pixel_edges instead of pixel_values #176

Conversation

yashrsharma44 commented May 25, 2019 • edited Loading

ghost commented May 25, 2019 • edited by ghost Loading

yashrsharma44 commented May 25, 2019

codecov bot commented May 28, 2019 • edited Loading

Codecov Report

pep8speaks commented May 29, 2019 • edited Loading

Comment last updated at 2019-07-31 15:41:29 UTC

yashrsharma44 commented May 29, 2019 • edited Loading

DanRyanIrish commented Jun 4, 2019

yashrsharma44 commented Jun 5, 2019

DanRyanIrish commented Jun 11, 2019

yashrsharma44 Jun 12, 2019

Choose a reason for hiding this comment

DanRyanIrish Jul 30, 2019

Choose a reason for hiding this comment

yashrsharma44 commented Jun 12, 2019 • edited Loading

yashrsharma44 commented Jul 25, 2019

DanRyanIrish commented Jul 26, 2019

yashrsharma44 commented Jul 26, 2019

DanRyanIrish commented Jul 26, 2019

yashrsharma44 commented Jul 26, 2019

DanRyanIrish Jul 30, 2019

Choose a reason for hiding this comment

yashrsharma44 Jul 30, 2019

Choose a reason for hiding this comment

DanRyanIrish commented Jul 30, 2019

yashrsharma44 commented Jul 30, 2019 • edited Loading

DanRyanIrish Jul 30, 2019

Choose a reason for hiding this comment

DanRyanIrish Jul 30, 2019

Choose a reason for hiding this comment

DanRyanIrish commented Jul 30, 2019

yashrsharma44 commented Jul 30, 2019

yashrsharma44 commented Jul 30, 2019

DanRyanIrish commented Jul 30, 2019

DanRyanIrish commented Jul 30, 2019 • edited by Cadair Loading

DanRyanIrish commented Jul 31, 2019

yashrsharma44 commented May 25, 2019 •

edited

Loading

ghost commented May 25, 2019 •

edited by ghost

Loading

codecov bot commented May 28, 2019 •

edited

Loading

pep8speaks commented May 29, 2019 •

edited

Loading

yashrsharma44 commented May 29, 2019 •

edited

Loading

yashrsharma44 commented Jun 12, 2019 •

edited

Loading

yashrsharma44 commented Jul 30, 2019 •

edited

Loading

DanRyanIrish commented Jul 30, 2019 •

edited by Cadair

Loading