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

Dst update #57

Merged
merged 8 commits into from
May 18, 2021
Merged

Dst update #57

merged 8 commits into from
May 18, 2021

Conversation

rstoneback
Copy link
Collaborator

@rstoneback rstoneback commented May 14, 2021

Description

Addresses #56

Fixed multi-day data loading issue. Also improved efficiency when loading multiple days, plus, I added metadata.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

start_date = dt.datetime(2001, 1, 1)
end_date = dt.datetime(2001, 12, 31)
dst = pysat.Instrument('sw', 'dst')
dst.load(date=start_date, end_date=end_date)
dst['dst'].plot(title=dst.meta['dst', dst.meta.labels.name], 
                ylabel=dst.meta['dst', dst.meta.labels.units])

image

Test Configuration

  • Operating system: Mac OS 10.15.4
  • Version number: Python 3.8.3

Checklist:

  • Make sure you are merging into the develop (not main) branch
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • Add a note to CHANGELOG.md, summarizing the changes

If this is a release PR, replace the first item of the above checklist with the
release checklist on the pysat wiki:
https://github.com/pysat/pysat/wiki/Checklist-for-Release

@rstoneback rstoneback changed the base branch from main to rc_v0.0.4 May 14, 2021 22:04
@rstoneback rstoneback requested a review from aburrell May 14, 2021 22:09
rstoneback and others added 2 commits May 14, 2021 17:15
Updated Dst tag to be informative, and easily allow for future additions of historic data from Kyoto.
@aburrell
Copy link
Member

I made some changes to partially address #59

Copy link
Member

@aburrell aburrell left a comment

Choose a reason for hiding this comment

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

I just made changes instead of requesting them. Tell me what you think @rstoneback

@rstoneback
Copy link
Collaborator Author

I just made changes instead of requesting them. Tell me what you think @rstoneback

Fair enough. Good catch on the missing return. Adding a 'noaa' tag certainly makes it easy to extend to other data sources in the future. My only hesitation is it makes instantiating slightly harder but it does also make code written today more robust against future changes. Works for me.

@rstoneback
Copy link
Collaborator Author

I have a draft pull over at pysatTutorials. My initial thinking was it could be good to have demos of the various pysatSpaceWeather instruments over there. Here is a simple example for Dst,
pysat/pysatTutorials#4

Then I wondered how many we want to maintain, plus what is the balance against examples in the documentation. Topics for a telecom perhaps and outside this pull. Still, leaving this note here given the specific connection to dst. It is the reason I found the bug.

@aburrell aburrell requested a review from jklenzing May 17, 2021 16:51
Copy link
Member

@jklenzing jklenzing left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Fixed the metadata re-assignment of the fill value.
@aburrell aburrell merged commit 1ab04bb into rc_v0.0.4 May 18, 2021
@aburrell aburrell deleted the dst_update branch May 18, 2021 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants