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

modified search metadata to return Astropy.Table #3950

Closed
wants to merge 7 commits into from

Conversation

DShivansh
Copy link
Contributor

Fixes #3941

@DShivansh DShivansh requested a review from a team as a code owner March 31, 2020 19:08
CHANGELOG.rst Outdated
@@ -196,6 +196,7 @@ Trivial/Internal Changes
- Corrected spelling of 'plotting' in timeseries method (changed 'ploting' to 'plotting'). (`#3429 <https://github.com/sunpy/sunpy/pull/3429>`__)
- Switched to "importlib_metadata" to get package version to speed up import of SunPy. (`#3449 <https://github.com/sunpy/sunpy/pull/3449>`__)
- Fix tests for `sunpy.data.data_manager` and ensure they are correctly executed with pytest. (`#3550 <https://github.com/sunpy/sunpy/pull/3550>`__)
- Modified the `search_metadata` function inside `sunpy.net.jsoc` to return the Astropy.Table instead of pandas dataframe. (`#3950 <https://github.com/sunpy/sunpy/pull/3950>`__)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use this changelog directly. You have to add an RST file into the changelogs folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nabobalis should I remove the change in CHANGELOG and only upload a file in changes.rst? or do we have to keep both?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this change and add a changelog file in to the changelog folder.

@nabobalis
Copy link
Contributor

What does the output look like now in terminal compared with before?

@nabobalis nabobalis added this to the 2.0 milestone Mar 31, 2020
@nabobalis nabobalis added the net Affects the net submodule label Mar 31, 2020
@DShivansh
Copy link
Contributor Author

New Output

T_REC                  T_OBS          ...       T_REC_epoch      
-------------------- ----------------------- ... -----------------------
2014-01-01T00:00:01Z 2014-01-01T00:00:08.57Z ... 1993.01.01_00:00:04_TAI
2014-01-01T00:00:13Z 2014-01-01T00:00:20.58Z ... 1993.01.01_00:00:04_TAI
2014-01-01T00:00:25Z 2014-01-01T00:00:32.57Z ... 1993.01.01_00:00:04_TAI
2014-01-01T00:00:37Z 2014-01-01T00:00:44.58Z ... 1993.01.01_00:00:04_TAI
2014-01-01T00:00:49Z 2014-01-01T00:00:56.57Z ... 1993.01.01_00:00:04_TAI
2014-01-01T00:01:01Z 2014-01-01T00:01:08.59Z ... 1993.01.01_00:00:04_TAI
2014-01-01T00:01:13Z 2014-01-01T00:01:20.59Z ... 1993.01.01_00:00:04_TAI
2014-01-01T00:01:25Z 2014-01-01T00:01:32.57Z ... 1993.01.01_00:00:04_TAI
2014-01-01T00:01:37Z 2014-01-01T00:01:44.58Z ... 1993.01.01_00:00:04_TAI
2014-01-01T00:01:49Z 2014-01-01T00:01:56.58Z ... 1993.01.01_00:00:04_TAI
2014-01-01T00:02:01Z 2014-01-01T00:02:08.58Z ... 1993.01.01_00:00:04_TAI

Old output

                                                           T_REC  ...              T_REC_epoch
aia.lev1_euv_12s[2014-01-01T00:00:01Z][304]  2014-01-01T00:00:01Z  ...  1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:00:13Z][304]  2014-01-01T00:00:13Z  ...  1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:00:25Z][304]  2014-01-01T00:00:25Z  ...  1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:00:37Z][304]  2014-01-01T00:00:37Z  ...  1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:00:49Z][304]  2014-01-01T00:00:49Z  ...  1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:01:01Z][304]  2014-01-01T00:01:01Z  ...  1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:01:13Z][304]  2014-01-01T00:01:13Z  ...  1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:01:25Z][304]  2014-01-01T00:01:25Z  ...  1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:01:37Z][304]  2014-01-01T00:01:37Z  ...  1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:01:49Z][304]  2014-01-01T00:01:49Z  ...  1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:02:01Z][304]  2014-01-01T00:02:01Z  ...  1993.01.01_00:00:04_TAI

[11 rows x 176 columns]

@nabobalis
Copy link
Contributor

Looking at that, I do think we should keep the first column from the old version.

@DShivansh
Copy link
Contributor Author

DShivansh commented Mar 31, 2020

first column is actually the index value, @nabobalis should I create a column named index in the astropy.Table and put index-values in it and do the indexing according to them?

@nabobalis
Copy link
Contributor

Sure.

CHANGELOG.rst Outdated Show resolved Hide resolved
Co-Authored-By: Nabil Freij <nabil.freij@gmail.com>
@pep8speaks
Copy link

pep8speaks commented Mar 31, 2020

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

Line 870:54: E251 unexpected spaces around keyword / parameter equals
Line 870:56: E251 unexpected spaces around keyword / parameter equals

Comment last updated at 2020-03-31 20:08:33 UTC

@nabobalis
Copy link
Contributor

What does the output look like now?

@DShivansh
Copy link
Contributor Author

output

 Index                    ...       T_REC_epoch      
------------------------------------------- ... -----------------------
aia.lev1_euv_12s[2014-01-01T00:00:01Z][304] ... 1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:00:13Z][304] ... 1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:00:25Z][304] ... 1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:00:37Z][304] ... 1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:00:49Z][304] ... 1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:01:01Z][304] ... 1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:01:13Z][304] ... 1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:01:25Z][304] ... 1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:01:37Z][304] ... 1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:01:49Z][304] ... 1993.01.01_00:00:04_TAI
aia.lev1_euv_12s[2014-01-01T00:02:01Z][304] ... 1993.01.01_00:00:04_TAI

@DShivansh DShivansh requested review from nabobalis and removed request for a team April 5, 2020 14:43
@nabobalis nabobalis removed their request for review April 9, 2020 09:55
@nabobalis
Copy link
Contributor

As things stand, this PR is good to go but we need to resolve how to deal with the breaking change or even if we need this function anymore.

These two questions are unresolved right now and as such, we can't merge this PR until then.

@DShivansh
Copy link
Contributor Author

@nabobalis should we raise some type of warning that indicates the user that we don't return pandas dataframe anymore instead it is returning the astropy df.

@nabobalis
Copy link
Contributor

The warning needs to have a way to turn it off and warning the user as they run the code after a update isn't really an advanced warning.

@DShivansh
Copy link
Contributor Author

DShivansh commented Apr 9, 2020

can we use some kind of flag in sunpyrc to turn off the warning once user knows about it?

@nabobalis
Copy link
Contributor

I don't have a good solution right now.

@Cadair Cadair removed this from the 2.0 milestone Apr 15, 2020
@dstansby
Copy link
Member

The way I have seen this sort of deprecation done previously is to add a return_astropy_table keyword argument, which defaults to None. If it is None, the code warns, telling the user can do return_astropy_table=True to silence the error, and if it is False it still uses the old behavior, but raises a warning saying that this functionality will be removed in a future version.

@dstansby dstansby added the [WIP] label Jul 5, 2020
@nabobalis
Copy link
Contributor

I think with the current GSoC project, we will probably remove this method as a result I will be closing this.

I want to thank @DShivansh for opening the PR and I am sorry we decided to change the goal posts.

@nabobalis nabobalis closed this Jul 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Affects the net submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Returning Astropy Table instead of Pandas DataFrame when using search_metadata() in JSOCClient.
5 participants