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

Dev 2021 #1

Open
wants to merge 47 commits into
base: master
Choose a base branch
from
Open

Dev 2021 #1

wants to merge 47 commits into from

Conversation

lbednarczyk
Copy link
Collaborator

No description provided.

@lbednarczyk lbednarczyk requested review from a team, shravanisamala and Ahm3dAlAli and removed request for a team June 30, 2021 20:36
app.R Outdated Show resolved Hide resolved
app.R Outdated Show resolved Hide resolved
app.R Outdated Show resolved Hide resolved
app.R Outdated Show resolved Hide resolved
app.R Outdated Show resolved Hide resolved
app.R Show resolved Hide resolved
Copy link
Collaborator

@JingFu1 JingFu1 left a comment

Choose a reason for hiding this comment

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

Recommended Priority: Urgent

According to the style guide, I think "ctb" is better than "aut"

JingFu1
JingFu1 previously approved these changes Jun 20, 2022
@neilhatfield neilhatfield removed the request for review from Phichchaya27 June 28, 2022 14:10
@Phichchaya27 Phichchaya27 requested review from JuddHe and sv101 June 28, 2022 17:10
sv101
sv101 previously approved these changes Jul 8, 2022
Copy link
Collaborator

@sv101 sv101 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!

Copy link
Member

@neilhatfield neilhatfield left a comment

Choose a reason for hiding this comment

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

I have a few changes I would like you to make and some things to investigate. On the whole the app appears to be in pretty decent shape.

DESCRIPTION

  • Change the title to Analysis of Covariance
  • Change the date to 2018-08-01
  • Change the Lifecycle to questioning
  • Change the Learning Outcome to "The student will learn to use interaction plots to decide whether interaction terms are significant in ANCOVA settings."

README.md

  • Spell out ANCOVA in the title
  • Add "# App Description" above the description that is there.

Files

  • Delete model2.csv as this does not appear to be used.
  • Delete all image files from the www folder which are no longer used. For example, arrow.gif, arrow.png, check.png, etc.
  • Delete the navcolor.css file from the www folder

app.R

  • Delete library(fontawesome) call and update the reference section accordingly.
  • Uncomment out library(sortable) as this package is needed
  • Add a section header for the game page in the UI definition (i.e., "### Game page ----")
  • Move the code that loads the data sets from between the UI and server to the labelled section just above the UI
  • Fix the formatting of the sendSweetAlert for the info button (remove the colon and add type = "info")
  • Change all instances of snake_case to camelCase to be aligned with the Style Guide
  • Check spelling and grammar. (E.g., "continous" should be "continuous"; "Select covariance" should be "Select covariate")
  • Change any tags$b to tags$strong
  • Put one space on both sides of symbols such as =, ==, and <-

Things to Investigate and Report Back On

  • I don't understand the presence of the Diagnostic Plots subsection of the Prereq's. I didn't see where those diagnostic plots are getting used elsewhere in the app. Did I miss something? If such plots aren't being used, then we should remove them from the prereq's
  • Currently, the app displays raw R output for the anova table. Should we convert this to a more polished version (like what I require in class)?
  • Why does the app fit models for the otter and diet data sets and then use predicted data for making the plots? Why aren't we just using the actual data for the plots?
  • Where does the output$p go? I can't find a corresponding UI part. Is this something that we need to add to the UI somewhere?
  • What is the role of maxScore and the commented out code towards the end of the server?

@Phichchaya27 Phichchaya27 dismissed stale reviews from sv101, NurulSyafiqah-pixel, and JingFu1 via 76794fd July 11, 2022 00:45
@luqije luqije self-requested a review May 24, 2023 20:08
Copy link

@luqije luqije left a comment

Choose a reason for hiding this comment

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

Report my reviews for "Things to Investigate and Report Bacnk On":

  • The diagnostic plots are not necessarily needed in Pre. Because later in the explore page and game page, we are not showing information related to these. Also, the app "Assumptions of ANOVA" offers information related to ANCOVA.
  • We should convert the R output to a polished version. Since not every user uses R when analyzing ANCOVA. Displaying the output using the ds version (used in the paper) will be better.
  • I think using a random dataset on Explore page can help the users more directly to see how things changes can affect the interaction with the p-value through the plot. However, in this case, we can add information about the meaning behind them. By practicing this, the users can explore the matching game on the Game page.
  • output$p is missing in the UI part of the Explore page. I believe the author is trying to show the feedback of what is the meaning of the p-value of each output with different datasets.
  • maxScore is missing in the UI part of the Game page. I think it's meant for calculating the total score of each trial.

Error and Suggestions:

  • The plots for the Diet dataset when choosing the variable Age with Gender and Pre-diet Weight with Gender are not showing with errors.
  • Add brake between each explanation in Pre would be helpful.

@Sburke26 Sburke26 self-requested a review June 1, 2023 18:23
Copy link

@Sburke26 Sburke26 left a comment

Choose a reason for hiding this comment

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

Here is what I noticed while reviewing:

There appears to be two README files.
The year badge in the README isn't updated properly.
The info pop-up does not have a circled i icon.
The app does not run with the object named "abChange". I had to change it to "ab_change" for it to run.
The prerequisites page could be improved to be more attractive and beneficial to the user.
In particular, removing the diagnostic plots is one way, as it appears they don't support what is covered in the app. Also, maybe switching to a collapsible tab for each type of analysis instead of a spaced out table would be better. When the window of the app is minimized, this spaced out table is hard to look at.
There also appears to be a few instances of commented out code. Some of which may come in handy if the app were to try to implement certain features (ex.score,).
There are also some issues with spacing in the code.

Edit:
Also I noticed there was a console message: "Warning in is.na(x) : is.na() applied to non-(list or vector) of type 'closure'".

Luqi Jiao Emanuele and others added 6 commits July 10, 2023 15:38
have trouble generating three different plot with different models.
Removed some old code related to downloading data.
Used lapply to generate plots and tables on matching game page.
rank_list, submit, and newChallenge are not working
Here is the updated code for moving the plots and tables into sortables.
updated the models, submit button is working, newChallenge button is working with minor thing needs to be fixed
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.

10 participants