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

review + edits (Ryo) #9

Closed
Ryo-N7 opened this issue Jan 15, 2022 · 10 comments
Closed

review + edits (Ryo) #9

Ryo-N7 opened this issue Jan 15, 2022 · 10 comments
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request

Comments

@Ryo-N7
Copy link
Collaborator

Ryo-N7 commented Jan 15, 2022

No description provided.

@Ryo-N7 Ryo-N7 added the documentation Improvements or additions to documentation label Jan 15, 2022
@Ryo-N7 Ryo-N7 self-assigned this Jan 15, 2022
@Ryo-N7 Ryo-N7 reopened this Jan 16, 2022
Ryo-N7 added a commit that referenced this issue Jan 16, 2022
@Ryo-N7
Copy link
Collaborator Author

Ryo-N7 commented Jan 16, 2022

  • Pizza plot vignette: Output of "Custom stats" version, BOTH single player + comparison don't look right.

single custom:
piz1

comparison custom:
piz2

@Ryo-N7
Copy link
Collaborator Author

Ryo-N7 commented Jan 16, 2022

  • all existing pictures of plots are screenshots of the plots in the plot panel rather than actual PNG outputs. (will fix this myself)

Ryo-N7 added a commit that referenced this issue Jan 16, 2022
@Ryo-N7
Copy link
Collaborator Author

Ryo-N7 commented Jan 16, 2022

main function code:

  • function arguments and general code not spaced out: brackets, if-else statements, function args, etc.
  • inconsistencies between using = and <- operators for assignment (also present in vignettes)
  • consistent indentation inside function code >>> Ctrl + I hotkey is your friend!

Ryo-N7 added a commit that referenced this issue Jan 16, 2022
@Ryo-N7
Copy link
Collaborator Author

Ryo-N7 commented Jan 16, 2022

  • in plot_pizza()

converted this:

        data_selected <- data %>%
        mutate(stat = ifelse(StatGroup == "Standard", "Attacking",
                             ifelse(StatGroup == "Shooting", "Attacking",
                                    ifelse(StatGroup == "Passing", "Possession",
                                           ifelse(StatGroup == "Pass Types", "Possession",
                                                  ifelse(StatGroup == "Goal and Shot Creation", "Possession",
                                                         ifelse(StatGroup == "Defense", "Defending",
                                                                ifelse(StatGroup == "Possession", "Possession",
                                                                       ifelse(StatGroup == "Miscellaneous Stats", "Defending", NA)))))))))

to this (using case_when() for multiple if-elses):

        data_selected <- data %>% 
          mutate(stat = case_when(
            StatGroup == "Standard" ~ "Attacking", 
            StatGroup == "Shooting" ~ "Attacking",
            StatGroup == "Passing" ~ "Possession",
            StatGroup == "Pass Types" ~ "Possession",
            StatGroup == "Goal and Shot Creation" ~ "Possession",
            StatGroup == "Defense" ~ "Defending",
            StatGroup == "Possession" ~ "Possession",
            StatGroup == "Miscellaneous Stats" ~ "Defending",
            TRUE ~ NA
          ))

@Ryo-N7
Copy link
Collaborator Author

Ryo-N7 commented Jan 16, 2022

  • there absolutely has to be a way to shorten plot_shot() but I think that deserves a completely separate branch/issue/PR itself

@Ryo-N7 Ryo-N7 added bug Something isn't working enhancement New feature or request labels Jan 16, 2022
@Ryo-N7
Copy link
Collaborator Author

Ryo-N7 commented Jan 16, 2022

now making sure tests still succeed...

  • tests don't work anymore because you're requiring data to contain "x", "y", "finalX", "finalY" now
  • you'll need to update the tests to reflect this change you made
  • also add error message and nudge user if they input a dataframe WITHOUT the above 4 variables
  • you should also add codecov & automated package testing with github actions >>> this can be a separate issue altogether

@Ryo-N7
Copy link
Collaborator Author

Ryo-N7 commented Jan 16, 2022

  • added lifecycle "experimental" and MIT license badge on README

Ryo-N7 added a commit that referenced this issue Jan 16, 2022
Ryo-N7 added a commit that referenced this issue Jan 16, 2022
- Add NEWS.md

references #9
Ryo-N7 added a commit that referenced this issue Jan 16, 2022
@Ryo-N7
Copy link
Collaborator Author

Ryo-N7 commented Jan 16, 2022

passflow is inverted??

Alba should be making passes from the LW, not the RW?

plot_passflow

@Ryo-N7 Ryo-N7 mentioned this issue Jan 16, 2022
@Ryo-N7
Copy link
Collaborator Author

Ryo-N7 commented Jan 16, 2022

  • added NEWS.md: basically update this before you merge a new PR and upgrade the package version number with all the updates you made to the main branch

abhiamishra pushed a commit that referenced this issue Jan 18, 2022
* edits to readme. references #9

* Add images for pizza plots. re-create image output for other plots in vignettes. references #9

* clean calculate_threat(). references #9

* add badges to readme. references #9

* - Update DESCRIPTION.
- Add NEWS.md

references #9

* spacing out code and minor edits for functions. references #9

* - update pkgdown website. references #9

* a few more edits to vignettes. references #9

* update pizza plot vignette with custom output images
@abhiamishra
Copy link
Owner

passflow is inverted??

Alba should be making passes from the LW, not the RW?

plot_passflow

Passflow along with heatmap and pass functions were fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants