Switch to astropy time and make SJI and SP more consistent

[tiagopereira]

Currently, the __repr__ of SJI is giving an exception because it calls isoformat(), while NDCube and SunPy now use astropy.time.Time. At the same time, extra_coords for both SJI and SP are now being populated with an object array of Time objects, which is rather useless (e.g. for time deltas). This PR fixes that, removes all calls to isoformat(), populates extra_coords with an Time object that is an array.

It also improves on some consistency between SJI and SP, fixes #113 (ended up going all lowercase after a few attempts at using it proved a significant burden to go uppercase). Also contributed to #100 by adding roll to meta of the spectrograph, and added roll to __repr__ of both spectrograph and SJI instances, since it is such an important keyword.

[DanRyanIrish]

Hi @tiagopereira. Thanks for this PR. This is an important change that we need. I had one suggestion about places we should and should not use parse_time.

[DanRyanIrish]

In sunpy 1.0, parse_time returns an astropy Time object. So I think IRISpy should continue to use parse_time so any change in sunpy is automatically picked up by irispy. However, the cases where a numpy array of individual parse_time outputs is being creating definitely need to be changed in the way you suggest, e.g. https://github.com/sunpy/irispy/pull/121/files#diff-c3a5b1ac9cb9013bd70b2db039f006b3R508

[tiagopereira]

I'm not convinced parse_time is better than Time, for two reasons:

• It adds an additional dependency
• A change in parse_time may break IRISPy since it will not be guaranteed that the new object can be added to a TimeDelta array

Maybe @Cadair has an opinion here?

[DanRyanIrish]

That's true. Those sorts of changes to sunpy would then require a change in irispy. The counter point would be that be that presumably irispy would want to be consistent with the sunpy's time representation. So if irispy used Time and sunpy changed, we would want to update irispy to stay consistent with that? This is a philosophical argument about how much separation we want to put between irispy and sunpy. So there isn't a strictly wrong answer.

Also, is irispy's only direct dependence on sunpy for parse_time? Maybe it is! But if not, then not using parse_time does not remove a dependency.

[DanRyanIrish]

Looks like we use sunpy.util.config and check_download_file from sunpy as well. Although these are to do with downloading files and so if we really wanted to remove the sunpy dependency we could probably find other solutions... I guess one question is, are any irispy users likely to want to avoid having sunpy in their stack?

[DanRyanIrish]

I spoke with @Cadair and he said he will provide a third perspective.

[tiagopereira]

Ok. I didn't mean that this would be the only dependency on SunPy, so my wording was a bit unfortunate. There are several other points where IRISPy depends on SunPy, in particular for plotting (e.g. coordinates, projections, and even colour maps). If there was a way to have parse_time make arrays of Time, that would probably be the best. But otherwise, if we want consistency between time in extra_coords and time in the meta attributes, I still think it makes more sense to go with astropy time.

[DanRyanIrish]

Oh! Perhaps we in fact agree then. In the latest versions of sunpy parse_time does exactly that. Give it an array of time stings and it returns a sinfle astropy Time object with multiple times rather than an array of individual time objects. So as long as the user has the latest version of sunpy they will get that behaviour. Is this what you want? Or is your preferred behaviour an array of Time objects?

Also, reading back at your previous comments, where is a Time object added to a TimeDelta in the code? Could you provide a link to the line?

[tiagopereira]

Time is added to TimeDelta when creating the time array in extra_coords, for the SJI and spectrograph. There is no sequence of time strings, only an array of seconds. So I thought it would be more logical to create an array of TimeDelta (which is essentially what that array is, time deltas).

[Cadair]

I am not 100% sure if I need to weigh in here, but in general if Time can parse the input there shouldn't be any reason to use parse_time over Time. When we transitioned over to astropy.time in SunPy we aimed to make parse_time(...) equivalent to Time(...) but accepting of more weird time formats.

[DanRyanIrish]

OK. Thanks for clarifying the philosophy of parse_time. This suggests to me that we should go with @tiagopereira's approach here.

[DanRyanIrish]

Regarding the comment from the imports section, I think here is an example where using parse_time should give the same output with the advantage that if the preferred sunpy time format is changed in the future and represented by parse_time, irispy does not need any changes.

[DanRyanIrish]

Hi @tiagopereira. In your work on this issue did you come across an error like this?

In [8]: sg = read_iris_spectrograph_level2_fits(filenames=glob.glob("iris_l2_20160213_142959_3660259533_raster/*")[0])                                                   
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-8-c91d30754292> in <module>
----> 1 sg = read_iris_spectrograph_level2_fits(filenames=glob.glob("iris_l2_20160213_142959_3660259533_raster/*")[0])

~/sunpy_dev/irispy/irispy/spectrograph.py in read_iris_spectrograph_level2_fits(filenames, spectral_windows, uncertainty, memmap)
    522                         "OBSID": hdulist[0].header["OBSID"],
    523                         "OBS_DESC": hdulist[0].header["OBS_DESC"],
--> 524                         "STARTOBS": parse_time(hdulist[0].header["STARTOBS"]),
    525                         "ENDOBS": parse_time(hdulist[0].header["ENDOBS"]),
    526                         "SAT_ROT": hdulist[0].header["SAT_ROT"] * u.deg,

~/miniconda3/envs/irispy-dev/lib/python3.7/site-packages/sunpy/time/time.py in parse_time(time_string, format, **kwargs)
    291         rt = Time.now()
    292     else:
--> 293         rt = convert_time(time_string, format=format, **kwargs)
    294 
    295     return rt

~/miniconda3/envs/irispy-dev/lib/python3.7/functools.py in wrapper(*args, **kw)
    825                             '1 positional argument')
    826 
--> 827         return dispatch(args[0].__class__)(*args, **kw)
    828 
    829     funcname = getattr(func, '__name__', 'singledispatch function')

~/miniconda3/envs/irispy-dev/lib/python3.7/site-packages/sunpy/time/time.py in convert_time_str(time_string, **kwargs)
    214             if ts is None:
    215                 continue
--> 216             return Time.strptime(ts, time_format, **kwargs) + time_delta
    217         except ValueError:
    218             pass

AttributeError: type object 'Time' has no attribute 'strptime'

I am using ndcube=1.2.1 (latest) which requires sunpy<=1.0.3. Here I am using sunpy=1.0.3. This bug however is fixed in sunpy=1.0.5 which is currently not allowed by ndcube.

[tiagopereira]

I am also using ndcube 1.2.1 and sunpy 1.0.3. But I haven't seen that error. Also did not try with a list of sp files, only with a single file at a time. Will test later, probably next week.

[DanRyanIrish]

On closer inspection, perhaps this bug is not fixed in sunpy=1.0.5...

[DanRyanIrish]

I am also using ndcube 1.2.1 and sunpy 1.0.3. But I haven't seen that error. Also did not try with a list of sp files, only with a single file at a time. Will test later, probably next week.

OK thaks for the info. I also was just using a single spectrograph file... OK. Have to figure out why I'm getting this error.

[Cadair]

which is currently not allowed by ndcube.

This is a bug in the conda package: conda-forge/ndcube-feedstock#15 the correct dep is sunpy >= 1.0.3, you can work around this by installing ndcube from pip until the package is rebuilt.

[DanRyanIrish]

What do we need to do get the conda package rebuilt?

Also, @Cadair, do you have any views on the use of parse_time vs. directly calling astropy.time.Time?

[Cadair]

Also, @Cadair, do you have any views on the use of parse_time vs. directly calling astropy.time.Time?

#121 (comment)

[Cadair]

I merged the conda-forge PR, so the fixed packages should be live soon.

[DanRyanIrish]

I merged the conda-forge PR, so the fixed packages should be live soon.

Thanks @Cadair!

[DanRyanIrish]

I'm happy to merge this. Unless there are any objects, I will do this in a couple hours.