Moved timeseries peek to plot()

[jeffreypaul15]

Description

Fixes #5066
Drawing parallels from GenericMap, I suppose this is along the lines of what we want?
This does override the base plot() method defined for TimeSeries though.

[pep8speaks]

Hello @jeffreypaul15! Thanks for updating this PR.

• In the file sunpy/timeseries/sources/eve.py:

Line 86:74: E228 missing whitespace around modulo operator

• In the file sunpy/timeseries/sources/lyra.py:

Line 88:79: E228 missing whitespace around modulo operator

Comment last updated at 2021-06-20 12:40:28 UTC

[jeffreypaul15]

@ayshih Is this something along the lines of what we want?

[nabobalis]

This looks like something we want, tho It will need to be done for each time series source since they all have their own peek method.

[jeffreypaul15]

This looks like something we want, tho It will need to be done for each time series source since they all have their own peek method.

Yeah, I just made the change for this one source to confirm if this is what we want. Shall I go about with the rest?

[nabobalis]

Sure

[jeffreypaul15]

@nabobalis I've added all the TimeSeries sources, but like I mentioned, this does override the plot() method defined in the base TimeSeries class, how would we want to handle that?

[nabobalis]

@nabobalis I've added all the TimeSeries sources, but like I mentioned, this does overwrite the plot() method defined in the base TimeSeries class, how would we want to handle that?

You mean it overloads it?

[jeffreypaul15]

You mean it overloads it?

No, I am overriding the plot method right

[nabobalis]

You mean it overloads it?

No, I am overriding the plot method right

I don't see the issue. What would be the problem.

[jeffreypaul15]

You mean it overloads it?

No, I am overriding the plot method right

I don't see the issue. What would be the problem.

We lose that functionality, basically the ability to call the base plot method. Is that fine?

[nabobalis]

You mean it overloads it?

No, I am overriding the plot method right

I don't see the issue. What would be the problem.

We lose that functionality, basically the ability to call the base plot method. Is that fine?

Don't see why not.

[jeffreypaul15]

What changelog would this PR require?

[nabobalis]

What changelog would this PR require?

Not sure. It feels kind of breaking to make plot do all this formatting but it doesn't actually break anything.

[jeffreypaul15]

Not sure. It feels kind of breaking to make plot do all this formatting but it doesn't actually break anything.

Shall I go with feature then?

[nabobalis]

Sure.

[jeffreypaul15]

Sure.

Need to word the changelog better, please make the required changes.

[jeffreypaul15]

@nabobalis I don't suppose extra tests are needed right? The test for peek() covers plot() so would we need separate tests for plot()?

[nabobalis]

@nabobalis I don't suppose extra tests are needed right? The test for peek() covers plot() so would we need separate tests for plot()?

No it should be ok.

[jeffreypaul15]

@nabobalis Anything else to be done in this PR?

[nabobalis]

It will need a review.

[nabobalis]

the colours aren't in accordance with when the user doesn't pass any axes as we're plotting each column of the dataframe separately on each of the axes passed, we could fix this by having a colours array but that is specific to this dataframe and I don't suppose that's right to do

Why?

I was under the assumption that this would be used for a variety of dataframes where the columns might exceed the default number of colours that we specify right?

Maybe? Just add the colours and cycle through them.

[jeffreypaul15]

the colours aren't in accordance with when the user doesn't pass any axes as we're plotting each column of the dataframe separately on each of the axes passed, we could fix this by having a colours array but that is specific to this dataframe and I don't suppose that's right to do

Why?

I was under the assumption that this would be used for a variety of dataframes where the columns might exceed the default number of colours that we specify right?

Maybe?

dataframe.plot() handles this by itself, including the rotation of the xticklabels so by plotting it separately we would need to manually set all of this

[nabobalis]

the colours aren't in accordance with when the user doesn't pass any axes as we're plotting each column of the dataframe separately on each of the axes passed, we could fix this by having a colours array but that is specific to this dataframe and I don't suppose that's right to do

Why?

I was under the assumption that this would be used for a variety of dataframes where the columns might exceed the default number of colours that we specify right?

Maybe?

dataframe.plot() handles this by itself, including the rotation of the xticklabels so by plotting it separately we would need to manually set all of this

I guess we add it then?

[jeffreypaul15]

I guess we add it then?

I've added the exact colours that are cycled, rotated the xticks to match and also updated the docs figure to be of a figsize=(20,10), the plot should now be rendered properly.

[jeffreypaul15]

@nabobalis A few sources (lyra and fermi_gbm) derive their title from the dataframe :
axes[0].set_title("LYRA ({0:{1}})".format(self.to_dataframe().index[0], TIME_FORMAT))
Do we still accept a default title argument here?

[nabobalis]

@nabobalis A few sources (lyra and fermi_gbm) derive their title from the dataframe :
axes[0].set_title("LYRA ({0:{1}})".format(self.to_dataframe().index[0], TIME_FORMAT))
Do we still accept a default title argument here?

Sure.

[nabobalis]

Can this be squash merged and rebased please?

[nabobalis]

I wanted to check what plot vs peek looked light right now.
I used the test data.
Each plot is plot then peek.

noaa_json_pre

noaa_pre_json

norh

norh plot broke for me but peek worked.

rhessi

esp

eve

fermi_gbm

goes

lyra

There are still some big differences? Is there anyway to resolve this?

[nabobalis]

Undoing my approval.

[jeffreypaul15]

@nabobalis These differences are resolved in this PR right?
Running all peek and plot methods from this PR look alike for me

[nabobalis]

I will double check I had checked out the correct code but this was from my attempt at testing this PR.

[jeffreypaul15]

I will double check I had checked out the correct code but this was from my attempt at testing this PR.

Sure, the current release version has the same differences that you've mentioned

[nabobalis]

I had sunpy installed twice in my env and this created the conflict.

[nabobalis]

Thanks again @jeffreypaul15