Update spectrogram.py #2034

prateekiiest · 2017-03-10T13:56:09Z

Formatted some attributes under class Spectogram

pep8speaks · 2017-03-10T13:56:19Z

Hello @prateekiiest! Thanks for updating the PR.

In the file sunpy/spectra/spectrogram.py, following are the PEP8 issues :

Line 763:39: E261 at least two spaces before inline comment
Line 765:62: E261 at least two spaces before inline comment
Line 766:48: E261 at least two spaces before inline comment
Line 789:32: E261 at least two spaces before inline comment
Line 791:64: E261 at least two spaces before inline comment
Line 902:9: E128 continuation line under-indented for visual indent
Line 903:9: E125 continuation line with same indent as next logical line
Line 903:9: E128 continuation line under-indented for visual indent
Line 956:59: E261 at least two spaces before inline comment
Line 957:70: E261 at least two spaces before inline comment
Line 974:9: E125 continuation line with same indent as next logical line
Line 974:9: E128 continuation line under-indented for visual indent
Line 1127:41: E261 at least two spaces before inline comment
Line 1229:101: E501 line too long (105 > 100 characters)
Line 1233:101: E501 line too long (103 > 100 characters)

Comment last updated on April 13, 2017 at 07:41 Hours UTC

prateekiiest

Fixed Pep8 issues

prateekiiest · 2017-03-10T14:11:26Z

issue #2033
ping @Cadair

nitinkgp23 · 2017-03-10T14:21:52Z

You should be checking for issues in your appveyor build. I guess, you have done a mistake in fixing pep8.

prateekiiest

Pep8 fixed issues created issue.
Fixed it :)

prateekiiest · 2017-03-10T15:03:18Z

Thanks @nitinkgp23 :)

prateekiiest

Some remaining issues fixed.

prateekiiest · 2017-03-16T09:11:34Z

I think this area is still under development as suggested by the docs. So an error is coming here.
In data = self.data[y_range, x_range] arguments should be of type int.

Cadair · 2017-03-16T10:14:30Z

have you merged master in?

prateekiiest · 2017-03-16T11:53:31Z

I didn't get you @Cadair

nitinkgp23 · 2017-03-16T13:30:23Z

@prateekiiest The master branch has been updated. Rebase your branch with master.

prateekiiest · 2017-03-16T13:32:48Z

Ok,
so some issue was fixed regarding this file I guess, a day ago

prateekiiest

Rebased branch

Cadair · 2017-04-05T16:55:11Z

sunpy/spectra/spectrogram.py

@@ -274,28 +274,28 @@ class Spectrogram(Parent):

    Attributes
    ----------
-    data : `~numpy.ndarray`
+    `~data` : `~numpy.ndarray`


this needs updating. The following conventions should be followed for docstrings:

Types (both built-in and things like numpy.ndarray) should be in single backticks and optionally have a ~ in front to only display the name of the class and not the whole thing, i.e ndarray over numpy.ndarray. (So built in types should not have a tilde).

Variable names that are not importable (like arguments to functions) should be in double backticks i.e:

``data``

prateekiiest

didnt give tilde as its optional

nabobalis · 2017-04-06T12:36:10Z

sunpy/spectra/spectrogram.py

+        if start:
+            start = int(start)
+        if end:
+            end = int(end)


Maybe I am confused, but instead of these if statements, would it not just be possible to int(start):int(end) on the return statement below?

@nabobalis
I think start and end variables are updated in the if statement.
I am not sure much, I need to go through the code for this.

These need to be moved into lines 1249 and 1262. The whole if statement is not needed.

Done @nabobalis
Why was it wrong in the previous case?

prateekiiest

checked

prateekiiest · 2017-04-06T12:55:43Z

sunpy/spectra/spectrogram.py

+        if start:
+            start = int(start)
+        if end:
+            end = int(end)


@nabobalis
I think start and end variables are updated in the if statement.
I am not sure much, I need to go through the code for this.

prateekiiest · 2017-04-06T13:03:22Z

is it now ok @Cadair ?

prateekiiest · 2017-04-07T11:33:06Z

should I rebase?

nabobalis · 2017-04-07T11:56:35Z

This failure seems to be an online test issue. I will rekick travis for kicks.

prateekiiest · 2017-04-07T12:05:53Z

Ok thanks :)

nabobalis · 2017-04-12T18:42:52Z

sunpy/spectra/spectrogram.py

        header for the image
-    instruments : str array
+    ``instruments`` : `str array`


These changes to the documentation are incorrect, they would need to be reverted. The `` do not need be on the right hand side and on the left only for individual types.

nabobalis · 2017-04-12T18:43:25Z

sunpy/spectra/spectrogram.py

@@ -274,28 +274,28 @@ class Spectrogram(Parent):

    Attributes
    ----------
-    data : `~numpy.ndarray`


The ~ will be needed here. It hides certain parts when the documentation is built.

nabobalis · 2017-04-12T18:44:46Z

sunpy/spectra/spectrogram.py

@@ -274,28 +274,28 @@ class Spectrogram(Parent):

    Attributes
    ----------
-    data : `~numpy.ndarray`


The ~ will be needed here. It hides certain parts when the documentation is built.

Is it ok now?

nabobalis · 2017-04-12T18:45:44Z

sunpy/spectra/spectrogram.py

+        if start:
+            start = int(start)
+        if end:
+            end = int(end)


These need to be moved into lines 1249 and 1262. The whole if statement is not needed.

prateekiiest

Added ~ and removed backticks on right hand side in some cases.

prateekiiest

If statement shifted to block 1243

prateekiiest · 2017-04-13T08:40:44Z

@nabobalis failing due to if block shifting?
The argument for int() can not be datetime

nabobalis · 2017-04-13T09:09:12Z

I wasn't clear in my comments. I didn't want to remove the code previous block of code. What needed to be moved was the int(start) and int(end) into the main if statement block for each. Otherwise, if start is None, int(None) will error.

So the changes to be made here would be:

start = self.time_to_x(start)
end = self.time_to_x(end)

would be

start = int(self.time_to_x(start))
end = int(self.time_to_x(end))

Also all the documentation changes made in the PR are wrong.

I would recommend starting again and just committing the spectrogram changes in a fresh PR.

prateekiiest · 2017-04-13T11:07:26Z

OK, I will send a new PR regarding this.
But I'm really packed up right now due to my semester exams. So I will not be able to devote much time untill 28th April.
After that I will be addressing to the PRs opened by me (probably 3 here) and one PR under website repo :)

Update spectrogram.py

a68c6ce

Update spectrogram.py

6e7a7c3

prateekiiest commented Mar 10, 2017

View reviewed changes

Update spectrogram.py

1e68d6c

prateekiiest commented Mar 10, 2017

View reviewed changes

Update spectrogram.py

523ab82

prateekiiest commented Mar 10, 2017

View reviewed changes

Update spectrogram.py

e7dea60

prateekiiest commented Mar 16, 2017

View reviewed changes

Cadair requested changes Apr 5, 2017

View reviewed changes

Update spectrogram.py

66567ba

prateekiiest commented Apr 6, 2017

View reviewed changes

nabobalis reviewed Apr 6, 2017

View reviewed changes

prateekiiest commented Apr 6, 2017

View reviewed changes

nabobalis reviewed Apr 12, 2017

View reviewed changes

Removed back_ticks on right hand side. Added tilde to numpy.

b54f727

prateekiiest commented Apr 13, 2017

View reviewed changes

Shifted If statement and removed previous if block

0dca7b9

prateekiiest commented Apr 13, 2017

View reviewed changes

Merge branch 'master' into patch-5

cbe1f96

Cadair closed this Apr 13, 2017

prateekiiest deleted the patch-5 branch April 13, 2017 11:20

Update spectrogram.py #2034

Update spectrogram.py #2034

Conversation

prateekiiest commented Mar 10, 2017

pep8speaks commented Mar 10, 2017 • edited

Comment last updated on April 13, 2017 at 07:41 Hours UTC

prateekiiest left a comment

Choose a reason for hiding this comment

prateekiiest commented Mar 10, 2017

nitinkgp23 commented Mar 10, 2017 • edited

prateekiiest left a comment

Choose a reason for hiding this comment

prateekiiest commented Mar 10, 2017

prateekiiest left a comment

Choose a reason for hiding this comment

prateekiiest commented Mar 16, 2017

Cadair commented Mar 16, 2017

prateekiiest commented Mar 16, 2017 • edited

nitinkgp23 commented Mar 16, 2017

prateekiiest commented Mar 16, 2017 • edited

prateekiiest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekiiest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekiiest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekiiest commented Apr 6, 2017

prateekiiest commented Apr 7, 2017

nabobalis commented Apr 7, 2017

prateekiiest commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekiiest left a comment

Choose a reason for hiding this comment

prateekiiest left a comment

Choose a reason for hiding this comment

prateekiiest commented Apr 13, 2017

nabobalis commented Apr 13, 2017

prateekiiest commented Apr 13, 2017 • edited

pep8speaks commented Mar 10, 2017 •

edited

nitinkgp23 commented Mar 10, 2017 •

edited

prateekiiest commented Mar 16, 2017 •

edited

prateekiiest commented Mar 16, 2017 •

edited

prateekiiest commented Apr 13, 2017 •

edited