Closed MemoOlv closed 2 years ago
Merging #29 (429c89c) into develop (bbb44d6) will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## develop #29 +/- ##
========================================
Coverage 95.23% 95.23%
========================================
Files 2 2
Lines 21 21
========================================
Hits 20 20
Misses 1 1
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update bbb44d6...429c89c. Read the comment docs.
En un momento lo reviso @MemoOlv
@nepito Sí, muchas gracias 😄
@nepito Sí, justo el siguiente paso es agregar la función para escribir el submission.
Me pongo a hacer los cambios y los voy subiendo. Gracias 👍🏽
¡Excelente trabajo, @MemoOlv! 👍🏾
@devarops Sí, suena una buena idea. Creo que quedaría mucho más limpio y bonito :+1:
Voy escribiendo el código a ver que tal
@devarops Listo 👍🏽
Las funciones quedaban exactamente como lo describiste en tu comentario. Tuve que hacer diferentes modificaciones en los archivos para que fueran coherentes y agregar nuevas pruebas como test_get_submission
en dummy_model
.
Quedó bastante pesado el pull request pero creo que el código quedó más limpio y ya se terminó de subir el último modelo.
¡Buen trabajo, @MemoOlv! 👍🏾
Quedó bastante pesado el pull request pero creo que el código quedó más limpio y ya se terminó de subir el último modelo.
Dado que ya terminamos de subir el último modelo, con este PR concluimos tu examen. 🏁
Ya que muchos de los cambios en este PR están relacionados con refactorización ♻️, en este caso haremos una excepción y aceptaremos que tenga más de 100 líneas.
Ya casi terminamos, @MemoOlv. Te pido paciencia en estas últimas revisiones. Considerando que este es el último PR, debemos aprovechar todas las oportunidades de refactorización que encontremos.
@devarops
Perfecto, me parece bien echarle otro ojo para refactorizar.
Voy viendo el código para detectar otras opciones de refactorizacion.
@devarops Que buen ojo 👀
Los cambios quedaron prácticamente igual a como los propusiste. Tuve que agregar unos cambios en las pruebas y en particular en tests_write_submission
. No me encantó la solución con muchos ´if´ pero al menos es funcional. Seguiré revisando otra forma de mejorar esa prueba.
Tal vez hoy no conteste tan rápido los mensajes, tengo que preparar unas cosas que entregar para trámites de mi maestría. Pero sin falta mañana puedo revisar bien las cosas.
En un momento lo hago esta revisión @MemoOlv y @devarops
@devarops @nepito Siguiendo la misma idea que:
Como el objetivo es tener confianza en las pruebas y no llenar de pruebas, lo que sigue es borrar esa prueba.
Estuve viendo que pruebas no eran necesarias y cuales no. Removí muchas de test_dummy_model
Este arroz ya se coció
Adding function that predicts age of pollos petreles using a power law model from
Longitud_ala
variablepredict_age_pollos_petrel_power_law
fuctiontest_predict_age_pollos_petrel_power_law
functiontest_predict_target_power_model
ytest_predict_target_linear_model