-
Notifications
You must be signed in to change notification settings - Fork 22
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/vmpy #6
Feature/vmpy #6
Conversation
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.
Posted some comments but I'm fine with merging and fixing/changing things later to keep things moving.
@@ -0,0 +1,215 @@ | |||
"""Very thin wrapper around the Strava API v3 |
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.
Just a question: I'm not too familiar with the Strava api (and stravalib) but why choose this over stravalib?
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've played with stravalib very long time ago and it used to load data into objects without an option to serialize those. I've heard that now all objects have to_dict() methods but I did not check. At the end the Strava API is so simple that it takes very concise request call followed by json.loads()
I have opened an issue to integrate Strava more tightly in sweat
#8
|
||
assert metrics.stress_score(norm_power, threshold_power, duration) == 100.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.
minor: too many newlines
logger = logging.getLogger(__name__) | ||
|
||
|
||
def power_duration_curve(arg, mask=None, value=0.0, **kwargs): |
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.
"mean_max_power_curve"?
This algorithm (my slower one) is also in sweat/algorithms/main.py so that has to be deleted. Is metrics a better place do put it?
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.
Yes, should be resolved. I've created an issue #9
return rv | ||
|
||
|
||
def normalized_power(arg, mask=None, value=0.0, **kwargs): |
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.
As discussed before: normalized power is a registered trademark.
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.
Will remove and replace #10
|
||
|
||
|
||
def wpk(power, weight): |
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'm not sure if this streams file makes sense to me. For example for this wpk method it would make sense for me to live in the metrics module. What do you think?
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.
Agree.
Posted some comments but I'm fine with merging and fixing/changing things later to keep things moving. |
I've added function as is into:
algorithm/streams.py
algorithm/metrics.py
io/strava.py
utils.py
I propose to proceed with refactoring using issues:
I did not touch the WDF interfaces and prefer to do it after we resolve 1 and 2.