Refactor curve fitting functions #63
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since these functions used in a pandas.GroupBy.apply() call, manipulating the index to calculate minute of the day can significantly reduce performance. Here we remove the _get_minute_of_day function and instead pass in the x values as a paramter. In addition to improving performance this makes the curve fitting code cleaner and substantially more general.
cwhanse
reviewed
Jul 20, 2020
Shows how to calculate numeric x-values for curve fitting to a time series.
These were old comments that are no longer necessary and don't quite match the current function.
Bring the comment up to date and provide a description of all parameters. This function has gotten complex enough that it needs a docstring-like comment to describe it.
Previously parameters for y-values (`day`) and x-values (`minutes`) were separated by the `fitfunc` parameter. It makes more sense for these two parameters to be adjacent.
cwhanse
approved these changes
Jul 21, 2020
pvanalytics/util/_fit.py
Outdated
| ---------- | ||
| data : Series | ||
| power or irradiance data. | ||
| x : Series |
There was a problem hiding this comment.
Suggested change
| x : Series | |
| x : array or Series |
There was a problem hiding this comment.
...unless you think this would require testing with array input type.
There was a problem hiding this comment.
I would feel better if we added tests with array input. Probably worth having some tests separate from the tests of orientation/tracker tests anyway.
There was a problem hiding this comment.
I think the best description for this type is numpy's 'array_like'. I'll change the type for x, but leave y as a Series.
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Change the description of the `x` parameter to curve fitting functions. This can be any array-like type, it doesn't have to be a Series. In fact we already pass non-series input (Int64Index).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Since these functions are used in a
pandas.GroupBy.apply()call, manipulating the index to calculate minute of the day can significantly reduce performance. Here we remove the_get_minute_of_day()function and instead pass in the x values as a parameter. In addition to improving performance this makes the curve fitting code cleaner and substantially more general.