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.

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

Merged
merged 6 commits into from
Mar 10, 2018
Merged

Feature/vmpy #6

merged 6 commits into from
Mar 10, 2018

Conversation

sladkovm
Copy link
Contributor

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:

  1. Data type handling - the issue is already created Discussion: input and output argument type casting #5
  2. Remove duplicate functionality

I did not touch the WDF interfaces and prefer to do it after we resolve 1 and 2.

@sladkovm sladkovm added the enhancement New feature or request label Mar 10, 2018
Copy link
Contributor

@AartGoossens AartGoossens left a 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
Copy link
Contributor

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?

Copy link
Contributor Author

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


Copy link
Contributor

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):
Copy link
Contributor

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?

Copy link
Contributor Author

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):
Copy link
Contributor

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.

Copy link
Contributor Author

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):
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree.

@AartGoossens
Copy link
Contributor

Posted some comments but I'm fine with merging and fixing/changing things later to keep things moving.

@sladkovm sladkovm merged commit 380487e into GoldenCheetah:master Mar 10, 2018
@sladkovm sladkovm deleted the feature/vmpy branch March 10, 2018 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants