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.

Sign up for GitHub

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

Jump to bottom

Remove panel #27

Merged
merged 34 commits into from
Jan 27, 2022
Merged

Remove panel #27

merged 34 commits into from
Jan 27, 2022

Conversation

rstoneback
Copy link
Collaborator

@rstoneback rstoneback commented Nov 17, 2021 •
edited

Description

Addresses #4, start on #3

Removes pandas.Panel from package.

Starts to switch code over to using xarray as main data format rather than something pandas based.

Tweaked general averaging code to reduce specificity with pandas. Started modifying computational_form to enable it's use in more areas.

The code is old, standards wise. Happy to sort that out as time goes on but a decent chunk of existing code may get deleted. I'd rather not make soon to be deleted code pretty. Overall plan is to reserve general comment and docstring improvements until after the transition to supporting xarray, as well as general nD binning support, is complete.

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide
instructions so we can reproduce. Please also list any relevant details for
your test configuration

  • Current suite of unit tests

Test Configuration:

  • MacOS 11.5
  • Python 3.8
  • Any details about your local setup that are relevant

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

@rstoneback
Copy link
Collaborator Author

Current implementation requires converting each DataFrame to a Dataset. Would likely be better if we could just make a Dataset in a single go. I've been having trouble with that though and wanted to get something up. Working solution > no solution.

@rstoneback rstoneback linked an issue Nov 17, 2021 that may be closed by this pull request
BUG: Panel deprecation #4
Closed
aburrell
aburrell reviewed Dec 8, 2021
View reviewed changes
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.

Partial review with some style things.

CHANGELOG.md Outdated
@@ -2,6 +2,9 @@
All notable changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/).

## [0.1.4] -
Copy link
Member

Choose a reason for hiding this comment

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

Add a placeholder for the date so it isn't forgotten.

pysatSeasons/_core.py Outdated
Comment on lines 6 to 7
"""
Repackages numbers, Series, or DataFrames
Repackages input data into xarray.Dataset
Copy link
Member

Choose a reason for hiding this comment

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

Fix docstring formatting

pysatSeasons/_core.py Outdated
Comment on lines 46 to 47
output[data.name] = data

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
output[data.name] = data
output[data.name] = data

pysatSeasons/_core.py Outdated
@@ -1,12 +1,13 @@
import pandas as pds
import xarray as xr


def computational_form(data):
Copy link
Member

Choose a reason for hiding this comment

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

Needs a more descriptive name. Maybe "to_xarray_dataset"?

pysatSeasons/_core.py Outdated
output[var] = xr.concat(data, 'pysat_binning')

elif isinstance(data[0], pds.DataFrame):
# Convert data to xarray
Copy link
Member

Choose a reason for hiding this comment

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

This is what the whole routine does, make comment more specific.

pysatSeasons/avg.py Outdated
Comment on lines 6 to 7
from __future__ import print_function
from __future__ import absolute_import
Copy link
Member

Choose a reason for hiding this comment

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

Remove

pysatSeasons/avg.py Outdated
@@ -23,77 +25,74 @@ def median1D(const, bin1, label1, data_label, auto_bin=True, returnData=False):
binX: array-like
List holding [min, max, number of bins] or array-like containing
bin edges, where X = 1, 2
labelX: string
labelX: str
Copy link
Member

Choose a reason for hiding this comment

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

Remove camelCase variable names

pysatSeasons/avg.py Show resolved Hide resolved
@jklenzing jklenzing mentioned this pull request Dec 8, 2021
Closed
3 tasks
@rstoneback
Copy link
Collaborator Author

Thanks for the comments @aburrell. Updated.

jklenzing
jklenzing reviewed Dec 22, 2021
View reviewed changes
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.

Looking good. Note that the version caps in setup.cfg need to be removed as well for when people are installing through pip.

pysatSeasons/_core.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
rstoneback and others added 3 commits January 13, 2022 13:30
@rstoneback @jklenzing @aburrell
Apply suggestions from code review
3094db9 
Co-authored-by: Jeff Klenzing <jklenzing@gmail.com>
Co-authored-by: Angeline Burrell <aburrell@users.noreply.github.com>
@rstoneback
STY: Removed extra line, flake8
794f376
@rstoneback
ENH: Removed version caps per reviewer help
91dd665
jklenzing
jklenzing approved these changes Jan 26, 2022
View reviewed changes
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! Thanks @rstoneback

@jklenzing
Copy link
Member

Just realized the changelog should probably show this as 0.2.0.

@rstoneback rstoneback merged commit bbb8271 into develop Jan 27, 2022
@rstoneback rstoneback deleted the remove_panel branch January 27, 2022 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aburrell aburrell aburrell left review comments

@jklenzing jklenzing jklenzing approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: Panel deprecation
3 participants
@rstoneback @jklenzing @aburrell