Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/implement daily bcsd #28
Feature/implement daily bcsd #28
Changes from 23 commits
2a35885
c1c31b8
18068b0
1eb3b39
990ddb2
9400f97
e281e8a
8c65c36
4a3dfeb
eb090b2
cdd0838
f6a12da
84d3aa2
48f5bc9
6daa1ba
8b6ef5a
1ae1a6d
1d9cdc5
1f58985
9036b9d
65fe1d6
1f2948e
bfc051c
3621ce4
7a5455f
d41ebe7
56162a6
38ac4ab
e75909d
3ada0eb
ed932a7
740befc
7e65248
acce768
65db340
fc85b9d
0e43bd0
b046af1
f9ee410
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which calendars will this support? I'm thinking it will be useful to check that the calendar of df.index is valid for this grouper method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's convert this to a proper
ValueError
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will be slightly faster if you allocate a numpy array via
np.full
and populate the values asresult[key] = group.mean().values[0]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be a DataFrame. In Pandas,
df.groupby(lambda x: x.month).mean() -> pd.DataFrame
ifdf
is a DataFrame.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should actually be a
Series
? It seems like if I make it a DataFrame, the time index gets wonkyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh nm, got this to work along with some other updates for your comment below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what should happen when obj is not a DataFrame?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually don't think we really need this function - I didn't end up using it in the NASA-NEX daily implementation and I think it's redundant with the testing that is in place now. seem reasonable @jhamman?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I don't think we need this anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool - just took this out.