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

BUG: replace reserved keyword freq with cadence #51

Merged
merged 3 commits into from
May 17, 2021
Merged

Conversation

jklenzing
Copy link
Member

@jklenzing jklenzing commented May 17, 2021

Description

Addresses #50

With the release of pysat 3.0.0, freq is now a reserved keyword. Changing to cadence for the purposes of this package. This is more descriptive, as the string is the desired measurement cadence.

Type of change

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

How Has This Been Tested?

  1. Import a module with a different cadence than the default ('1s') using the cadence keyword`.
  2. Load the data
  3. Check that data is at desired cadence

Example:

import pysat
import pysatMissions
test = pysat.Instrument(inst_module=pysatMissions.instruments.pysat_ephem, cadence='10s')
test.load(2019, 1)

Note that instrument registration is broken due to #49. This will be fixed in a separate pull.

Test Configuration:

  • Mac OS X 10.15.7
  • python 3.8.2

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

@jklenzing jklenzing changed the title Sty/freq BUG: replace reserved keyword freq with cadence May 17, 2021
@jklenzing jklenzing added the bug Something isn't working label May 17, 2021
@jklenzing jklenzing added this to the 0.3.0 release milestone May 17, 2021
@jklenzing jklenzing linked an issue May 17, 2021 that may be closed by this pull request
@jklenzing jklenzing marked this pull request as ready for review May 17, 2021 18:46
@jklenzing
Copy link
Member Author

NOTE: I've added a custom set of unit tests under the testing class to check that cadence works as expected. These tests should be run in addition to the standard inherited tests.

@jklenzing jklenzing requested a review from aburrell May 17, 2021 18:47
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.

Looks fine to me. I'd have used load_freq instead of cadence, but have no problem with the fix.

@jklenzing jklenzing merged commit 48a57bb into develop May 17, 2021
@jklenzing jklenzing deleted the sty/freq branch May 17, 2021 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: freq keyword not passing through
2 participants