Magnetogram overplotting gallery example #2037

Punyaslok · 2017-03-13T15:38:08Z

Note : This requires #1852 to be merged.

To-do :

Fix the past years' files are tarred issue in SRS Client and SRS reader. #1852
Remove unnecessary comments

@Cadair @dpshelio

pep8speaks · 2017-03-13T15:38:12Z

Hello @Punyaslok! Thanks for updating the PR.

In the file examples/gallery/magnetogram_active_regions.py, following are the PEP8 issues :

Line 63:76: W291 trailing whitespace

In the file sunpy/net/fido_factory.py, following are the PEP8 issues :

Line 61:21: E128 continuation line under-indented for visual indent
Line 402:1: E101 indentation contains mixed spaces and tabs
Line 402:1: W191 indentation contains tabs
Line 403:1: E101 indentation contains mixed spaces and tabs

In the file sunpy/net/vso/vso.py, following are the PEP8 issues :

Line 294:101: E501 line too long (108 > 100 characters)
Line 454:79: W291 trailing whitespace
Line 471:79: W291 trailing whitespace
Line 474:9: E731 do not assign a lambda expression, use a def

Comment last updated on August 04, 2017 at 09:13 Hours UTC

Cadair · 2017-03-27T17:12:08Z

The build has failed due to a bug which is fixed in master. I suggest you rebase on master than then rebase again when we merge the SRS code.

Cadair · 2017-03-27T22:19:26Z

examples/gallery/magnetogram_active_regions.py

+##############################################################################
+# Start by importing the necessary modules.
+
+from sunpy.net import Fido, attrs as a


the sunpy import should be in the second block:

stdlib external sunpy

Cadair · 2017-03-27T22:19:39Z

examples/gallery/magnetogram_active_regions.py

+# Download the files.
+
+downresp = Fido.fetch(results)
+#downresp = ['/home/punya/sunpy/data/hmi_m_45s_2017_01_01_00_02_15_tai_magnetogram.fits']


Cadair · 2017-03-27T22:20:12Z

examples/gallery/magnetogram_active_regions.py

+##############################################################################
+# Download the files.
+
+downresp = Fido.fetch(results)


downresp is not a great variable name downloaded_files is probably better.

Cadair · 2017-03-27T22:20:23Z

examples/gallery/magnetogram_active_regions.py

+# Download the SRS file.
+
+srs_results = Fido.search(a.Time(start_time, end_time), a.Instrument('SOON'))
+#srs_downresp = ['/home/punya/sunpy/data/20170101SRS.txt']


Cadair · 2017-03-27T22:21:09Z

examples/gallery/magnetogram_active_regions.py

+##############################################################################
+# Now we make the plot.
+
+##############################################################################


each of these blocks should have code in it so this should only be one row of comments

Cadair · 2017-03-27T22:21:42Z

examples/gallery/magnetogram_active_regions.py

+
+smap = sunpy.map.Map(file_name)
+
+im = smap.plot()


do you use im again? if not remove it.

Cadair · 2017-03-27T22:22:51Z

also the gallery examples should be very strict with PEP 8, so make the tireless bot happy ;) (for that file)

dpshelio · 2017-04-02T11:33:23Z

examples/gallery/magnetogram_active_regions.py

+# We will select a small time range to avoid downloading too many files.
+
+start_time = day + " 00:01:00"
+end_time = day + " 00:02:00"


I'm not too happy to add strings starting with spaces. In this case I may suggest to put the date and time together as strings, eg:

start_time = "2017-01-01 00:01:00" end_time = "2017-01-01 00:02:00"

or to do it using datetime like

start_time = datetime.datetim(2017, 1, 1, 0, 1, 0) end_time = start_time + datetime.timedelta(0, 60)

For this example I probably would go with the first option, but I don't see the point to have strings with dates when python treats dates properly (it's just reminds me too much to idl)

The strings could start with T instead.

dpshelio · 2017-04-02T11:34:29Z

examples/gallery/magnetogram_active_regions.py

+# now `srs_table` contains an astropy table.
+
+srs_table = srs.read_srs(srs_downresp[0])
+print (srs_table)


not spaces after print please ;)

dpshelio · 2017-04-02T11:38:19Z

examples/gallery/magnetogram_active_regions.py

+# Add the numbers as labels for each point.
+
+for i, num in enumerate(numbers):
+	ax.annotate(num, (lngs[i].value, lats[i].value), xycoords=ax.get_transform('heliographic_stonyhurst'))


num should be formatted in 10000 after 2002-06-15.

Punyaslok · 2017-07-23T08:27:27Z

Note : Now this also requires #2170 to be merged LOL

😇

Cadair · 2017-07-26T17:10:49Z

it's rebase o'clock git rebase all the things.

… add magnetogram_active_regions.py !

Cadair · 2017-08-03T08:03:02Z

examples/gallery/magnetogram_active_regions.py

+# Now to download and read the SRS file.
+# Download the SRS file.
+
+srs_results = Fido.search(a.Time(start_time, end_time), a.Instrument('SOON'))


should we change the instrument here to SRS_table?

Cadair · 2017-08-03T08:04:01Z

examples/gallery/magnetogram_active_regions.py

+# We only need the rows which have 'ID' = 'I' or 'IA'.
+
+if 'I' in srs_table['ID'] or 'IA' in srs_table['ID']:
+    srs_table = srs_table[np.logical_or(


I would not line break this here in a gallery example. If you really want to line break I would do it after a ,.

dpshelio

But concerned about too many a[0] when we suppose to get only one file

dpshelio · 2017-08-04T08:27:33Z

examples/gallery/magnetogram_active_regions.py

+##############################################################################
+# As the `fetch` method returns a list, our filename is its first element.
+
+file_name = downloaded_files[0]


I don't get all the indexing if in result we have chosen only the first file, why then we call the next one downloaded_files and then index that one again to get its filename.

dpshelio · 2017-08-04T08:27:57Z

examples/gallery/magnetogram_active_regions.py

+
+##############################################################################
+# Now to download and read the SRS file.
+# Download the SRS file.


These two sentences are redundant...

dpshelio · 2017-08-04T08:30:54Z

examples/gallery/magnetogram_active_regions.py

+srs_downloaded_files = Fido.fetch(srs_results)
+
+##############################################################################
+# We get one SRS file per day. So we pass the filename into the SRS reader. So


we get one file per day... but we queried only for one day... does this mean we are getting more than one?

yes, we get one file per day, but [0] is necessary here because read_srs expects str, bytes or os.PathLike object, not list.

Punyaslok force-pushed the magnetogram_gallery_example branch from daa936a to 53f6443 Compare March 27, 2017 22:18

Cadair requested changes Mar 27, 2017

View reviewed changes

dpshelio reviewed Apr 2, 2017

View reviewed changes

nabobalis added the Examples Affects the Example Gallery label Apr 12, 2017

Punyaslok force-pushed the magnetogram_gallery_example branch 2 times, most recently from dbf720f to 2145122 Compare June 24, 2017 07:45

Punyaslok force-pushed the magnetogram_gallery_example branch from 0fd05c0 to 32a41fb Compare July 12, 2017 21:00

Cadair added this to Nice to Haves in SunPy 0.8 Jul 17, 2017

Punyaslok added 9 commits August 3, 2017 09:00

Magnetogram-ARs overplotting example

4813d8b

Changes made corresponding to comments

a829afd

Changed to handle to I or IA entries in table.

d40de48

pep8 fixes

e295259

pep8 fixes

4cf6065

Made minor corrections

3a1f537

Changed to add fido indexing. Changed date to a day with more ARs git…

44c5b9f

… add magnetogram_active_regions.py !

No ARs handling added

f9afea6

pep8 fixes

412adf9

Cadair force-pushed the magnetogram_gallery_example branch from 9aa48a9 to 412adf9 Compare August 3, 2017 08:01

Cadair requested changes Aug 3, 2017

View reviewed changes

Cadair added the Needs Review Needs reviews before merge label Aug 3, 2017

Minor changes

21e847a

Cadair approved these changes Aug 4, 2017

View reviewed changes

dpshelio approved these changes Aug 4, 2017

View reviewed changes

Punyaslok added 2 commits August 4, 2017 14:22

redundant line remove

6a38f6a

Minor changes made

7875993

Cadair merged commit 5837de4 into sunpy:master Aug 4, 2017

Cadair moved this from Nice to Haves to Finished in SunPy 0.8 Aug 4, 2017

nabobalis removed the Needs Review Needs reviews before merge label Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Magnetogram overplotting gallery example #2037

Magnetogram overplotting gallery example #2037

Punyaslok commented Mar 13, 2017 •

edited

pep8speaks commented Mar 13, 2017 •

edited

Cadair commented Mar 27, 2017

Cadair Mar 27, 2017

Cadair Mar 27, 2017

Cadair Mar 27, 2017

Cadair Mar 27, 2017

Cadair Mar 27, 2017

Cadair Mar 27, 2017

Cadair commented Mar 27, 2017

dpshelio Apr 2, 2017

Cadair Apr 2, 2017

dpshelio Apr 2, 2017

dpshelio Apr 2, 2017

Punyaslok commented Jul 23, 2017

Cadair commented Jul 26, 2017

Cadair Aug 3, 2017

Punyaslok Aug 3, 2017

Cadair Aug 3, 2017

Punyaslok Aug 3, 2017

dpshelio left a comment

dpshelio Aug 4, 2017

Punyaslok Aug 4, 2017

dpshelio Aug 4, 2017

Punyaslok Aug 4, 2017

dpshelio Aug 4, 2017

Punyaslok Aug 4, 2017

Magnetogram overplotting gallery example #2037

Magnetogram overplotting gallery example #2037

Conversation

Punyaslok commented Mar 13, 2017 • edited

pep8speaks commented Mar 13, 2017 • edited

Comment last updated on August 04, 2017 at 09:13 Hours UTC

Cadair commented Mar 27, 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

Choose a reason for hiding this comment

Cadair commented Mar 27, 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

Punyaslok commented Jul 23, 2017

Cadair commented Jul 26, 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

dpshelio 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Punyaslok commented Mar 13, 2017 •

edited

pep8speaks commented Mar 13, 2017 •

edited