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

Simplified user access to the sourceID variable in the HelioviewerClient. #2926

Merged
merged 10 commits into from Mar 16, 2019

Conversation

Projects
None yet
5 participants
@Naman9639
Copy link
Contributor

commented Feb 5, 2019

Description

Fixes #2878

@pep8speaks

This comment has been minimized.

Copy link

commented Feb 5, 2019

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

Line 81:101: E501 line too long (121 > 100 characters)
Line 137:101: E501 line too long (121 > 100 characters)
Line 174:101: E501 line too long (130 > 100 characters)
Line 175:101: E501 line too long (129 > 100 characters)
Line 194:101: E501 line too long (121 > 100 characters)
Line 233:101: E501 line too long (103 > 100 characters)
Line 448:52: E231 missing whitespace after ','
Line 451:29: E127 continuation line over-indented for visual indent
Line 451:71: W291 trailing whitespace
Line 454:1: W293 blank line contains whitespace
Line 454:1: W391 blank line at end of file

Line 149:1: E302 expected 2 blank lines, found 1
Line 150:22: E231 missing whitespace after ','
Line 150:26: E231 missing whitespace after ','
Line 150:31: E231 missing whitespace after ':'
Line 150:34: E231 missing whitespace after ','
Line 150:36: E231 missing whitespace after ','
Line 150:45: E231 missing whitespace after ','
Line 150:49: E231 missing whitespace after ','
Line 150:54: E231 missing whitespace after ':'
Line 150:57: E231 missing whitespace after ','
Line 150:59: E231 missing whitespace after ','
Line 150:68: E231 missing whitespace after ','
Line 150:72: E231 missing whitespace after ','
Line 150:77: E231 missing whitespace after ':'
Line 150:80: E231 missing whitespace after ','
Line 150:82: E231 missing whitespace after ','
Line 151:44: E231 missing whitespace after ','
Line 151:49: E231 missing whitespace after ','
Line 151:88: E231 missing whitespace after ','
Line 151:92: E231 missing whitespace after ','
Line 152:1: W293 blank line contains whitespace
Line 152:1: W391 blank line at end of file

Line 217:1: E302 expected 2 blank lines, found 1
Line 220:101: E501 line too long (105 > 100 characters)
Line 220:106: W291 trailing whitespace

Comment last updated at 2019-03-16 14:17:39 UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented Feb 5, 2019

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

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Feb 5, 2019

We probably need a unit test for the new function.

@nabobalis nabobalis added this to the 1.0 milestone Feb 5, 2019

Show resolved Hide resolved sunpy/net/helioviewer.py Outdated
Show resolved Hide resolved sunpy/net/helioviewer.py Outdated
Show resolved Hide resolved sunpy/net/helioviewer.py Outdated
Show resolved Hide resolved sunpy/net/helioviewer.py Outdated
Show resolved Hide resolved sunpy/net/helioviewer.py Outdated
Show resolved Hide resolved sunpy/net/tests/test_helioviewer.py Outdated
Show resolved Hide resolved sunpy/net/tests/test_helioviewer.py Outdated
Show resolved Hide resolved sunpy/net/helioviewer.py Outdated
Show resolved Hide resolved changelog/2926.trivial.rst Outdated

@nabobalis nabobalis changed the title Issue2878 Simplified user access to the sourceID variable in the HelioviewerClient. Feb 5, 2019

Show resolved Hide resolved sunpy/net/helioviewer.py Outdated

@Naman9639 Naman9639 force-pushed the Naman9639:issue2878 branch from 14ffa61 to 84da02a Feb 6, 2019

@Naman9639

This comment was marked as off-topic.

Copy link
Contributor Author

commented Feb 8, 2019

Are these changes ok?

Show resolved Hide resolved sunpy/net/helioviewer.py Outdated
Show resolved Hide resolved sunpy/util/util.py Outdated
Show resolved Hide resolved sunpy/util/util.py Outdated
Show resolved Hide resolved sunpy/util/util.py Outdated
Show resolved Hide resolved sunpy/util/util.py Outdated
Show resolved Hide resolved changelog/2926.trivial.rst Outdated

@Naman9639 Naman9639 force-pushed the Naman9639:issue2878 branch from b070510 to ba16fdb Feb 8, 2019

Show resolved Hide resolved sunpy/util/util.py Outdated
Show resolved Hide resolved sunpy/net/helioviewer.py Outdated
Show resolved Hide resolved sunpy/net/helioviewer.py Outdated

@Naman9639 Naman9639 force-pushed the Naman9639:issue2878 branch from e6d742f to 555a90b Feb 8, 2019

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Feb 9, 2019

What is this test codecov/patch for? And why is it failing?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Feb 9, 2019

It checks for line coverage of our code through our test suite. It failed because it seems that this pull request lowers the amount of line coverage.

Sometime this happens due to fails in the CI or the git history becomes tangled instead of an actual issue.

Show resolved Hide resolved changelog/2926.trivial.rst Outdated
Show resolved Hide resolved changelog/2926.trivial.rst Outdated
Show resolved Hide resolved changelog/2926.trivial.rst Outdated
Show resolved Hide resolved sunpy/net/helioviewer.py Outdated
@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Feb 14, 2019

The pip one is due to a change in master that would require this PR to be rebased. So we can ignore that.

The windows one seems to be an issue with numpy update in anaconda. We can ignore that too.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Feb 14, 2019

Ah ok

The pip one is due to a change in master that would require this PR to be rebased. So we can ignore that.

The windows one seems to be an issue with numpy update in anaconda. We can ignore that too.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Feb 22, 2019

Um should I change anything?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Feb 22, 2019

Nothing that I am aware of. Unfortunately, you will just have to wait until we get another review.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Feb 22, 2019

I needed some help writing my proposal

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Feb 22, 2019

I would suggest raising that in the chat room instead of this PR.
Just to remind that, that we are not aware if we have even been accepted by GSoC yet (February 26, 2019).

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Feb 22, 2019

Ok I will wait till 26th

Show resolved Hide resolved sunpy/net/helioviewer.py
Show resolved Hide resolved sunpy/net/helioviewer.py Outdated
Show resolved Hide resolved sunpy/util/util.py
Show resolved Hide resolved sunpy/util/util.py Outdated
Show resolved Hide resolved sunpy/util/util.py Outdated
Show resolved Hide resolved sunpy/net/helioviewer.py Outdated
Show resolved Hide resolved sunpy/net/helioviewer.py Outdated

1. filepath = hv.download_jp2('2012/07/03 14:30:00', observatory='SDO', instrument='HMI', detector=None, measurement='continuum')
2. filepath = hv.download_jp2('2012/07/03 14:30:00', observatory='SDO', measurement='continuum')
3. filepath = hv.download_jp2('2012/07/03 14:30:00', instrument='HMI', measurement='continuum')

This comment has been minimized.

Copy link
@nabobalis

nabobalis Mar 6, 2019

Contributor

We had a discussion on this and in hindsight, this should be in the documentation string for the functions to tell people they can do this instead of the changelog.

This comment has been minimized.

Copy link
@Naman9639

Naman9639 Mar 6, 2019

Author Contributor

oh ok I will do that

This comment has been minimized.

Copy link
@nabobalis

nabobalis Mar 7, 2019

Contributor

We need to add some comments to each doc string that uses sourceID, telling users that you can do this.

This comment has been minimized.

Copy link
@Naman9639

Naman9639 Mar 8, 2019

Author Contributor

Like telling the user that in place of source ID we can use the partial key to retrieve the source ID?

This comment has been minimized.

Copy link
@nabobalis

nabobalis Mar 8, 2019

Contributor

Yes

@Naman9639 Naman9639 force-pushed the Naman9639:issue2878 branch from ec309c7 to 619701e Mar 6, 2019

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Mar 8, 2019

Yes.

@Naman9639 Naman9639 force-pushed the Naman9639:issue2878 branch from dbeb678 to 0789529 Mar 16, 2019

@Naman9639

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Sorry for the delay. I was having my exams. I will be active again

@@ -169,19 +165,16 @@ def download_jp2(self, date, observatory=None, instrument=None, detector=None,
>>> import sunpy.map
>>> from sunpy.net import helioviewer
>>> hv = helioviewer.HelioviewerClient() # doctest: +REMOTE_DATA
>>> # We can just pass the observatory and measurement or instrument and measurement to get the value for source ID.

This comment has been minimized.

Copy link
@nabobalis

nabobalis Mar 16, 2019

Contributor

I would not put this in the example section. But above in the description.

@Naman9639 Naman9639 force-pushed the Naman9639:issue2878 branch from 5e5add2 to 2661145 Mar 16, 2019

Naman9639 and others added some commits Mar 16, 2019

@@ -77,6 +78,8 @@ def get_closest_image(self, date, observatory=None, instrument=None,
detector=None, measurement=None, source_id=None):
"""
Finds the closest image available for the specified source and date.
We can use `observatory` and `measurement` or `instrument` and `measurement` to get the value for source ID which

This comment has been minimized.

Copy link
@nabobalis

nabobalis Mar 16, 2019

Contributor

Actually in fact, I wonder if wrapping this in a rst .. note:: would be better.

This comment has been minimized.

Copy link
@Naman9639

Naman9639 Mar 16, 2019

Author Contributor

Yeah I guess that would be better. I will change it

This comment has been minimized.

Copy link
@Naman9639

Naman9639 Mar 16, 2019

Author Contributor

I need to build the docs to see if it is getting rendered properly right?

This comment has been minimized.

Copy link
@nabobalis

nabobalis Mar 16, 2019

Contributor

Yes.

@Cadair

Cadair approved these changes Mar 16, 2019

@Cadair Cadair merged commit 0032890 into sunpy:master Mar 16, 2019

16 checks passed

ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: pip-install Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 85.8%)
Details
codecov/project 85.82% (+0.01%) compared to a557fc1
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy Build #20190316.7 succeeded
Details
sunpy.sunpy (Linux_36_Conda_offline) Linux_36_Conda_offline succeeded
Details
sunpy.sunpy (Linux_36_offline) Linux_36_offline succeeded
Details
sunpy.sunpy (Linux_37_online) Linux_37_online succeeded
Details
sunpy.sunpy (Windows_36_offline) Windows_36_offline succeeded
Details
sunpy.sunpy (macOS_37_offline) macOS_37_offline succeeded
Details
@Cadair

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

I got a little hasty with the merge button there, feel free to open another PR to tidy up the docs 😄

@Naman9639

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

I got a little hasty with the merge button there, feel free to open another PR to tidy up the docs

No issues. I will open another one

@Naman9639 Naman9639 referenced this pull request Mar 16, 2019

Merged

Docs clean up #2995

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.