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.

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

PR for getting pixel_edges along with pixel_values in axis_world_coords #174

Merged
merged 15 commits into from
May 23, 2019

Conversation

yashrsharma44
Copy link
Member

@yashrsharma44 yashrsharma44 commented May 22, 2019

This PR adds a new functionality for returning pixel_edges along with pixel_values from axis_world_coords.
Things to TODO:

  • Add tests
  • Documentation
  • Add changelog

Ping @DanRyanIrish @Cadair

Fixes part of the stagnant PR #172

@ghost
Copy link

ghost commented May 22, 2019

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

@pep8speaks
Copy link

pep8speaks commented May 22, 2019

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-05-23 14:06:40 UTC

ndcube/ndcube.py Outdated Show resolved Hide resolved
ndcube/ndcube.py Show resolved Hide resolved
ndcube/ndcube.py Outdated Show resolved Hide resolved
ndcube/ndcube.py Outdated Show resolved Hide resolved
ndcube/ndcube.py Show resolved Hide resolved
ndcube/ndcube.py Outdated Show resolved Hide resolved
ndcube/ndcube.py Outdated Show resolved Hide resolved
ndcube/utils/cube.py Show resolved Hide resolved
ndcube/ndcube.py Show resolved Hide resolved
@DanRyanIrish
Copy link
Member

Hi @yashrsharma44. This looks good. I left a couple minor formatting comments and then assuming this passing the online tests, I think it's ready to approve.

@yashrsharma44
Copy link
Member Author

Let me add some tests for the helper functions.

@yashrsharma44
Copy link
Member Author

Hey @DanRyanIrish , do we need to mention about this new feature in the docs, or shall this go unnoticed?

@DanRyanIrish
Copy link
Member

DanRyanIrish commented May 23, 2019

I think this would be great to add this to the docs. It may be a very useful feature for users. It doesn't have to be too long. Just a couple sentences at the end of the Coordinate Transformation section saying some like:
"By default ~ndcube.NDCube.axis_world_coords returns the coordinates at the center of each pixel. However, the pixel edges can be obtained by setting the edges kwarg to True."
Then provide an example showing the output:

>>> my_cube.axis_world_coords(edges=True)
output...

@DanRyanIrish
Copy link
Member

The tests are failing because there are some blank lines containing spaces:
ndcube/ndcube.py:329:1: W293 blank line contains whitespace
ndcube/utils/cube.py:164:1: W293 blank line contains whitespace
ndcube/utils/cube.py:169:1: W293 blank line contains whitespace
ndcube/utils/cube.py:173:1: W293 blank line contains whitespace
ndcube/utils/cube.py:188:1: W293 blank line contains whitespace

@yashrsharma44
Copy link
Member Author

The blank spaces that I added between the docstrings are creating the pep8 issue

@DanRyanIrish
Copy link
Member

Right. But it's not that the lines exist. It's that they contain spaces. You just need to delete those spaces without removing the empty line and that should fix it.

@yashrsharma44
Copy link
Member Author

Sure, I would make those changes.

@Cadair Cadair added this to the 1.2 milestone May 23, 2019
changelog/174.feature.rst Outdated Show resolved Hide resolved
@DanRyanIrish DanRyanIrish merged commit d40b7c8 into sunpy:master May 23, 2019
@DanRyanIrish
Copy link
Member

Great work @yashrsharma44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants