Skip to content

Conversation

aburrell
Copy link
Member

@aburrell aburrell commented Jun 30, 2022

Description

Moves the custom OMNI routines to an instrument module, in accordance with newer pysat style. Also fixes (temporary?) bug in the clean routine when dealing with array-like metadata.

This is a draft because this needs more work that I don't have time to do:

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality
    to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Updated unit tests, using OMNI locally.

Test Configuration

  • Operating system: OS X Big Sur
  • Version number: Python 3.8
  • Any details about your local setup that are relevant: updated use_header branch of pysat.

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
  • Update zenodo.json file for new code contributors

aburrell added 3 commits June 30, 2022 09:54
Moved the OMNI HRO local functions to a separate instrument module.  Also fixed a bug in the clean routine, which now allows for array-like inputs.
Updated the changelog with a summary of the commits.
Fixed imports and cleaned up PEP8 violations.
@jklenzing jklenzing self-assigned this Aug 3, 2022
@jklenzing jklenzing added this to the 0.0.4 release milestone Aug 3, 2022
@jklenzing
Copy link
Member

Thinking about development priorities, I'd prefer to pull in the deprecations for the v0.0.4 release and push the tests out to v0.1.0.

Comment on lines +198 to +201
def deprecated(func):
"""Warn users that function has moved locations.
Decorator function for deprecation warnings.
Copy link
Member

Choose a reason for hiding this comment

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

🎵 Soon may a decorator come 🎵
🎶 To deprecate functions that shouldn't be run 🎶
🎵 And when the testing is done 🎵
🎶 We'll take our leave and go 🎶

@jklenzing jklenzing marked this pull request as ready for review August 9, 2022 15:13
@jklenzing
Copy link
Member

jklenzing commented Aug 9, 2022

@aburrell, I've taken this about as far as I can for now. I don't feel comfortable writing the unit tests for the other omni functions because I don't know how to check if the current algorithms are correct. I see three possible routes forward:

  1. Review and merge now to get this into v0.0.4. TST: specialized OMNI HRO routines need testing #34 can be handled later.
  2. Write unit tests to ensure current output does not change. Rely on reviews to determine if functions are producing correct output.
  3. Hand the branch over to someone else.

I should add that the way I'm running the deprecation tests, each function is actually run, so it looks like the coverage is increased. If we take option 1, I should probably update that to avoid confusion in the future.
Realized I could use an empty instrument and scan for expected KeyError to evaluate more quickly.

jklenzing and others added 3 commits August 9, 2022 13:43
Fixed the bug that only allowed OMNI downloads for the first of the month.
@jklenzing jklenzing merged commit 0d4deca into develop Sep 7, 2022
@jklenzing jklenzing deleted the omni_hro_reorg branch September 7, 2022 15:53
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