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

Added 3D elevation plot - Fixes #47 #48

Merged
merged 7 commits into from
Feb 4, 2022

Conversation

BalintKomjati
Copy link
Contributor

Running strava::plot_3D will generate the following demo plot:

image

I added a new tracklog gpx with hilly scene to make the demo more appealing. I put it into a separate folder (gpx/mtb) because process_data() reads in all tracks from a folder without explicit reference to the file, only an arbitrary id number, making it uncertain if the demo will read the hilly gpx.

@marcusvolz
Copy link
Owner

@BalintKomjati This looks amazing! I will test on my own data in the next few days.

@BalintKomjati
Copy link
Contributor Author

UX and macOS tests failed - i will try to fix it in my forked repo.

@BalintKomjati
Copy link
Contributor Author

Hello @marcusvolz and @datawookie!
Could you please help me fixing the install checks?
I am sure this is caused by the new dependency packages needed for the 3D plot above.

Windows passed before, i managed to fix the Ubuntu (release) run by adding the necessary OS components into the check.yaml of the R packages 'terra' and 'magick'.

I guess the same is needed for MacOS but i have no experience with this OS...

And I am out of ideas for the Ubuntu (devel) which fails because "there is no package called ‘terra’".

Putting me into the right direction with hints could also be a great help :)

thanks

Bálint

@marcusvolz
Copy link
Owner

@datawookie would you mind taking a look at this if you are around and have time? I am not fully up to speed on the new workflows yet.

@BalintKomjati
Copy link
Contributor Author

BalintKomjati commented Dec 29, 2021

@datawookie would you mind taking a look at this if you are around and have time? I am not fully up to speed on the new workflows yet.

Hi @datawookie did you have the chance to review this PR?
https://github.com/BalintKomjati/strava/actions/runs/1492301193
Here are the check logs showing Win and Ubuntu are passed but Ubuntu(devel) and MacOS failed for some reason I could not figure out ...

Thanks in advance!!
Bálint

@datawookie
Copy link
Collaborator

Hi @BalintKomjati, I've sorted out the tests. See BalintKomjati#2 on your repository. If you accept those changes then we can accept this PR too. Thanks, Andrew.

@BalintKomjati
Copy link
Contributor Author

Thank you so much @datawookie! I accepted the PR in my repository.

@marcusvolz
Copy link
Owner

@BalintKomjati @datawookie Let me know if there's anything you need from me to get this into the main repo. I'd be keen to test out the plot_3D function on my own data.

@datawookie
Copy link
Collaborator

Hi @BalintKomjati, I've just run the GitHub Action with the package checks and it appears to be failing on the Windows platform. Could you please take a look at that and try to resolve the issue? Shout if you need any help. Thanks, Andrew.

@BalintKomjati
Copy link
Contributor Author

Hi @datawookie @marcusvolz!
To be fairly honest I kind of lost track of what is happening because different tests fail depending on the time we ran them. My best bet is that this is due to new releases of the imports / depends.

Yes, lately only Win fails (https://github.com/BalintKomjati/strava/actions/runs/1741925794). Google searches mainly just suggest to restart / reinstall everything or change the order of imports but nothing about the root cause of the error message:

Error: Error: Function found when exporting methods from the namespace 'raster' which is not S4 generic: 'all.equal'

On a separate branch I tried a few not-so-educated guesses but i could not resolve the issue.
https://github.com/BalintKomjati/strava/tree/plot3d-fix-checks

So @datawookie I would be grateful to have you take a look as well!

@BalintKomjati
Copy link
Contributor Author

Hi All!

I just rerun the checks on @datawookie 's last commit 804488e and it is all passed now :)

See: https://github.com/BalintKomjati/strava/actions/runs/1741925794

@marcusvolz can you pls rerun the test for this PR and if all good accept?

Cheers,
Bálint

Copy link
Collaborator

@datawookie datawookie left a comment

Choose a reason for hiding this comment

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

Looks good and all tests pass! 🎉

@datawookie
Copy link
Collaborator

Thanks for your patience and perseverance with this, @BalintKomjati. Merging right now!

@datawookie datawookie merged commit 3283718 into marcusvolz:master Feb 4, 2022
@marcusvolz
Copy link
Owner

Thank you both! Looking forward to testing this out!

@BalintKomjati
Copy link
Contributor Author

It is my pleasure :)

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

Successfully merging this pull request may close these issues.

3 participants