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

Add support for descent #54

Open
dblock opened this issue Mar 14, 2022 · 4 comments
Open

Add support for descent #54

dblock opened this issue Mar 14, 2022 · 4 comments

Comments

@dblock
Copy link
Owner

dblock commented Mar 14, 2022

Descent is Ascent + (Start Elevation - End Elevation). Start Elevation and End Elevation can be obtained from AltitudeStream.

@benCoomes
Copy link

benCoomes commented Apr 7, 2022

Hi @dblock I'd like to take a shot at this, as I need to learn Ruby and am familiar with the Strava API. However, I can't think of a good spot to put a total_elevation_loss method. The main problem is that calculating elevation loss requires data from two different endpoints (Stream for start and end elevation and Activity for total elevation gain). Are there any examples in the library of models generated through multiple resource requests? I couldn't find any.

I can see two options where the user is responsible for getting the supplemental data:

  • Add total_elevation_loss methods to the Elevation mixin. The altitude stream is a required parameter.
  • Or, create an AltitudeStream class which is a subclass of Stream. Add total_elevation_loss methods to it, which have a required total elevation gain parameter.
    • Alternatively, AltitudeStream methods could compute total elevation loss/gain directly from the stream's data. I am not sure if this will match what Strava calculates exactly.

Which of these would fit best with the library? Or, is this probably something that users need to be responsible for computing?

@dblock
Copy link
Owner Author

dblock commented Apr 8, 2022

I think we should think about it backwards. Let's start with the right interfaces at the model level first, then we can rethink where things go if they need to be in mixins. Mixins are just ways to reuse common functionality.

So, which model(s) need(s) total_elevation_loss? Start there and implement them in any way that works, and we can refactor into mixins later.

@benCoomes
Copy link

Okay, that makes sense. Just thinking about what each model class represents, activity_total, activity, explorer_segment, lap, route, segment, and split could all have a total_elevation_loss property. And, all except explorer_segement use the elevation mixin already. Since explorer_segement doesn't currently have any elevation properties, do you think they should all be added? I don't think adding descent by itself makes sense.

I'll start with the others though, and see what implementing a total_elevation_loss property looks like.

@dblock
Copy link
Owner Author

dblock commented Apr 13, 2022

Okay, that makes sense. Just thinking about what each model class represents, activity_total, activity, explorer_segment, lap, route, segment, and split could all have a total_elevation_loss property. And, all except explorer_segement use the elevation mixin already. Since explorer_segement doesn't currently have any elevation properties, do you think they should all be added? I don't think adding descent by itself makes sense.

If they exist on that model, yes.

I'll start with the others though, and see what implementing a total_elevation_loss property looks like.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants