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

Feedback #1

Open
wants to merge 17 commits into
base: feedback
Choose a base branch
from
Open

Feedback #1

wants to merge 17 commits into from

Conversation

github-classroom[bot]
Copy link
Contributor

@github-classroom github-classroom bot commented May 11, 2023

👋! GitHub Classroom created this pull request as a place for your teacher to leave feedback on your work. It will update automatically. Don’t close or merge this pull request, unless you’re instructed to do so by your teacher.
In this pull request, your teacher can leave comments and feedback on your code. Click the Subscribe button to be notified if that happens.
Click the Files changed or Commits tab to see all of the changes pushed to main since the assignment started. Your teacher can see this too.

Notes for teachers

Use this PR to leave feedback. Here are some tips:

  • Click the Files changed tab to see all of the changes pushed to main since the assignment started. To leave comments on specific lines of code, put your cursor over a line of code and click the blue + (plus sign). To learn more about comments, read “Commenting on a pull request”.
  • Click the Commits tab to see the commits pushed to main. Click a commit to see specific changes.
  • If you turned on autograding, then click the Checks tab to see the results.
  • This page is an overview. It shows commits, line comments, and general comments. You can leave a general comment below.
    For more information about this pull request, read “Leaving assignment feedback in GitHub”.

Subscribed: @RexCaecos

Copy link

@eliocamp eliocamp left a comment

Choose a reason for hiding this comment

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

Te hice comentarios. El código está bien así que son más que nada sobre la interpretación de los resultados.

informe.Rmd Outdated
Comment on lines 28 to 33
IP <- gsub(".*? ([[:digit:]])", "\\1", system("ipconfig", intern=T)[grep("IPv4", system("ipconfig", intern = T))])
if (IP == "10.2.29.7") {
Sys.setenv("https_proxy"="proxy.jus.gov.ar:8080/")
Sys.setenv("http_proxy"="proxy.jus.gov.ar:8080/")
options(internet.info = 0) #valor 0 o 1 para ver la comunicación con internet
}

Choose a reason for hiding this comment

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

Y esto? Si es algo específico de la configuración de tu computadora, quizás no debería estar en el código que voy a correr yo u otres colaboradores.

Copy link
Contributor

Choose a reason for hiding this comment

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

Es específico de la pc laboral, pero le puse como condición que verifique que sea mi IP interna laboral que es fija, entonces en otros equipos no debería ejecutarse. No encontré todavía como dejar la variable de entorno del proxy guardada en un .ini para que la ejecute al iniciar rstudio. Probé guardando en file.edit('~/.Renviron') pero no funcionó.

informe.Rmd Outdated
Comment on lines 22 to 24
library(DescTools)
library(tidyr)
library(datos) # por si las dudas

Choose a reason for hiding this comment

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

Estos paquetes no se usan en el código así que podés no cargarlos.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sí, gracias, olvidé limpiar el código.

informe.Rmd Outdated
Comment on lines 116 to 118
```{r echo=FALSE}
summarise(vinos, puntos_min = min(puntos, na.rm = TRUE), puntos_max = max(puntos, na.rm = TRUE))
```

Choose a reason for hiding this comment

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

¿Qué interpretás de esto?

Copy link
Contributor

Choose a reason for hiding this comment

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

Todavía me faltan algunos conceptos de estadística. Solo se me ocurrió, en base al máximo, mínimo y promedio, que los precios altos tienen mucha dispersión, porque se alejan mucho de la media.

informe.Rmd Outdated
```
También llamó la atención la cantidad de variedades de vinos, ``r variedades``.

Choose a reason for hiding this comment

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

Por qué?

Copy link
Contributor

Choose a reason for hiding this comment

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

Simplemente pensé que eran demasiadas variedades, por lo que supuse que había algunas escritas de diferente forma, pero las listé y ordené alfabéticamente y al parecer no.

informe.Rmd Outdated
p <- vinos |>
filter(precio < 250) |>
ggplot( aes(x=precio, fill=precio)) +
geom_histogram(binwidth=3, show.legend = FALSE, fill=rainbow(83))

Choose a reason for hiding this comment

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

Ojo acá que estás hardcodeando el número de barras cuando ponés rainbow(83). Lo que en realidad querés hacer acá es mapear el fill al precio del vino y usar una escala de arcoíris.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lo que pasa es que intuitivamente entendí que para que el gráfico mostrara información relevante había que desestimar los valores muy altos, entonces con criterio "a ojo" lo acoté a 250. Pero al usar el fill arcoiris me tomaba el rango entero (hasta 3300) y en definitiva quedaba todo en rojo. No encontré otra forma, y las funciones de graficación no terminan de quedarme claras.

informe.Rmd Outdated
prop_no_encontrados <- round(colSums(is.na(vinos))/nrow(vinos),4)*100
show(prop_no_encontrados)
```
Podemos encontrar bastantes valores faltantes, pero únicamente en las columnas de nombre (``r prop_no_encontrados[2]``%), region_1 (``r prop_no_encontrados[6]``%) y region_2 (``r prop_no_encontrados[6]``%).

Choose a reason for hiding this comment

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

Qué interpretás de esto?

Copy link
Contributor

Choose a reason for hiding this comment

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

Las columnas región 1 y región 2 contienen información adicional, por lo que es esperable que en muchos casos tengan valores nulos, pero no encontré una explicación para que falten tantos en la columna nombre. Podría haberlo considerado como anomalía.

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.

2 participants