[IV-22-23] Objetivo 4 PMP_IV #23

MauronMP · 2022-11-09T15:22:05Z

…adida pytest-tap en .toml

…rerías para jupyter

…proyectos en jupyer

…Añadidas dependencias con poetry

JJ

El problema principal es que tienes que seguir las mejores prácticas para colocar los ficheros y testearlos directamente sin notebook

MauronMP · 2022-11-10T15:37:50Z

El problema principal es que tienes que seguir las mejores prácticas para colocar los ficheros y testearlos directamente sin notebook

Ya he hecho que no se hagan los tests con los notebooks, sino en ficheros '.py', funcionando correctamente

JJ

Os dije desde el principio que tenéis que implementar vosotros la lógica de negocio, para testear todo lo que necesitéis e incluir sólo las funciones que requiera vuestro proyecto. Por tanto, no puedes usar ni pandas ni numpy, al menos en este milestone, donde lo importante es que tengáis la funcionalidad adecuada.
Además, CSV es una biblioteca core en Python, así que tampoco necesitar cargar con esas librerías sólo para leer un CSV.
La elección de herramientas parece adecuada, pero el desarrollo del código no ha seguido la metodología correcta.

JJ · 2022-11-11T19:13:01Z

pmp_iv/segmentacionCSV/Datos.py

+class Datos():
+
+    def __init__(self):
+        self.df = pd.read_csv('data/incendiosForestales.csv')


Aparentemente, gran parte o todo este fichero está desarrollado refiriéndote a issues que hablan sólo de elegir bibliotecas.
Es funcionalidad nueva. Siendo así, tienes que crear issues y referirte a las HUs correspondientes.

…#21, #22, #24, #7

MauronMP · 2022-11-14T13:10:10Z

Buenas, he cambiado la clase Datos para que use el módulo csv de python y no use pandas, de igual modo los test se han cambiado, creado un issue para este problema y funcionando correctamente. Listo para revisar

JJ

Aparte de las cosas adicionales que te comento, ¿filtrar es toda la parte de la lógica de negocio que vas a testear? ¿No habría que meter algo realmente de cálculo? Os advertí desde el principio que la lógica de negocio había que programarla, y testearla... En concreto, no parece que sólo filtrar satisfaga de alguna forma #7 o de qué forma contribuye al mismo.

JJ · 2022-11-14T13:15:22Z

docs/Test_runner.md

+Vamos a partir de las valoraciones de [esta página](https://snyk.io/advisor/python). Que hace una comparativa de cada librería para un proyecto para distintos lenguajes, en este caso python. La manera de evaluar cómo de bueno es un paquete es por:
+- Popularidad.
+- Mantenimiento.
+- Seguridad.


¿Qué tiene que ver la seguridad con un test runner? ¿Cómo la mides?

JJ · 2022-11-14T13:15:52Z

iv.yaml

@@ -9,3 +9,6 @@ automatizar:
  fichero: tasks.py
  orden: invoke

+test: 
+  - tests/test_Jupyter.py


Este fichero ya no existe.

JJ · 2022-11-14T13:16:17Z

tests/test_Datos.py

@@ -0,0 +1,22 @@
+from hamcrest import *


Mira en errores frecuentes de la semana 10 lo que se dice sobre nombres de ficheros.

JJ · 2022-11-14T13:16:47Z

pyproject.toml

-python = ">=3.6" 
+python = ">=3.8" 
+pyhamcrest = "^2.0.4"
+matplotlib = "^3.6.2"


¿Esto lo estás usando en producción?

JJ · 2022-11-14T13:18:50Z

pmp_iv/segmentacionCSV/Datos.py

+            filter_column.append(data[i][column])
+        return filter_column
+
+    def diagramaDispersion(self,data,area,image_name):


Una clase "Datos" no puede incluir un diagrama de dispersión. Tampoco hay ninguna historia de usuario que lo pida, y menos para este milestone.
Si lo quieres usar para ilustrar el README.md, simplemente mételo en un subdirectorio con scripts, pero lo mejor sería mantenerlo fuera del repositorio (ya que no añade valor a ninguna historia de usuario y no lo estás testando)

JJ · 2022-11-14T13:20:43Z

pmp_iv/segmentacionCSV/Datos.py

+class Datos():
+
+    def size_of_CSV(self):
+        with open('data/incendiosForestales.csv', 'r', newline='') as file:


¿En qué issue dice que necesitas saber el tamaño del CSV? Y, sobre todo, ¿por qué es esto una función y no se mete en el constructor?

JJ · 2022-11-14T13:21:17Z

pmp_iv/segmentacionCSV/Datos.py

+
+    def by_date_property(self, mes, dia, propiedad=None):
+        mes_dia = []
+        with open('data/incendiosForestales.csv', 'r', newline='') as csvfile:


¿Estás abriendo el fichero en cada función?
¿Seguro que no necesitas plantear un problema en un issue sobre este tema?

JJ · 2022-11-14T13:21:58Z

tests/test_Datos.py

+import os
+
+def test_sizeOf_CSV():
+    assert_that(Datos().size_of_CSV(), equal_to(517))


No debes usar literales en ningún test. En el momento que cambies el fichero esto va a petar.

…dos #29

JJ

Por segunda vez, sólo se puede usar código propio en este objetivo, como se advirtió en el objetivo cero

JJ · 2022-11-16T06:21:28Z

pmp_iv/forest_prediction/eda.py

+        return list_property
+
+    def weather(self):
+        all_propierties = []


typo: propierties

JJ · 2022-11-16T06:21:57Z

pmp_iv/forest_prediction/model_building.py

@@ -0,0 +1,47 @@
+from sklearn.preprocessing import StandardScaler


¿Qué parte de "sólo puedes usar código propio" no has entendido?

MauronMP · 2022-11-17T10:51:48Z

@JJ

JJ

Ninguna de las objeciones es grave, pero acumuladas dejan el código en un estado que podría no ser adecuado para progresar a las siguientes fases.
Adicionalmente, no has puesto explícitamente los criterios que has usado para primero buscar y luego seleccionar ninguna de las herramientas, de acuerdo con lo que explicamos en clase. Implícitamente parece que estás usando la puntuación de snyk, pero no parece que eso sólo deba ser un criterio.

JJ · 2022-11-18T06:30:11Z

tasks.py


+@task
+def jupyter(c):


Esto ya no lo necesitas

JJ · 2022-11-18T06:31:50Z

pmp_iv/forest_prediction/correlacion_area.py

+from pmp_iv.forest_prediction.eda import *
+from math import *
+
+class correlacion_dato_area():


Por favor, infórmate y sigue las buenas prácticas. Para empezar, la clase y el fichero deberían llamarse igual.
Una clase siempre se llama por los datos que contiene, no por lo que hace.

JJ · 2022-11-18T06:33:25Z

pmp_iv/forest_prediction/correlacion_area.py

+        return self.denominador
+
+
+    def calculo_coeficiente(self):


Estás haciendo un montón de cálculos sobre algo que no varía, puesto que se lee de un fichero.

JJ · 2022-11-18T06:33:50Z

pmp_iv/forest_prediction/correlacion_area.py

+class correlacion_dato_area():
+
+    def __init__(self, propiedad):
+        self.X = EDA().by_property(propiedad)


¿Estás leyendo el mismo fichero 2 veces?

JJ · 2022-11-18T06:34:47Z

tests/test_pmp_iv.py

+import pmp_iv.models.fecha 
+import pmp_iv.models.estado 
+import pmp_iv.models.fwi 
+import pmp_iv.enums.day 


Sigue sin ser una buena práctica esta organización del repositorio, ya te lo dije.

JJ · 2022-11-18T06:36:17Z

tests/test_pmp_iv.py

+    assert_that(pmp_iv.models.estado.Estado(10,10,9,3,200).temperature, less_than_or_equal_to(MAXIMA_TEMPERATURA))
+
+def test_month_filter():
+    filtrado_mes = EDA().by_date_property(MES_AGOSTO,DIA_LUNES)


Estás leyendo EDA múltiples veces. Deberías intentar usar las tres fases; Arrange, Act and Assert, y leer todo lo necesario (en Python se les llama Fixtures) en una primera fase (todas las bibliotecas de aserciones lo tienen)

and assert #31

for #26

MauronMP · 2022-11-21T11:12:57Z

Buenas @JJ, el test usa fixtures, Arrange, Act and Assert. He actualizado el directorio pmp_iv/ y no creo varias instancias de la clase EDA. Solicito revisión.

JJ

El objetivo 4 estaría bien, pero ahora no está bien el 3. Tienes que poner una tarea que compruebe la sintaxis.

JJ · 2022-11-21T11:23:23Z

pmp_iv/models/coordenada.py

-from pmp_iv.config.model_validation import ModelValidation
-from pmp_iv.utils.validation import Validation
+from pmp_iv.validaciones.model_validation import ModelValidation
+from pmp_iv.validaciones.validation import Validation


Una vez más, la jerarquía de ficheros debe reflejar la de objetos. No veo por qué no tienen que estar estos en el directorio principal, incluso en el mismo fichero.

JJ · 2022-11-21T11:24:27Z

pmp_iv/models/correlacion_area.py

+        return self.numerador
+
+    def den_coeficiente_x_y(self):
+        cuadrado_x = 0


Ninguna de estas funciones son pesadas como para "retrasar" su uso. Poniéndolas en una función estás forzando a que se llamen múltiples veces, en vez de simplemente calcularlas en el momento que estén los datos disponibles, que es en la construcción.

JJ · 2022-11-21T11:26:30Z

pmp_iv/models/eda.py

+            self._datos_csv = self.get_data_values()
+            self._area = self.by_property('area')
+
+    def get_data_values(self):


Igual que antes, mira errores frecuentes semana 11

JJ · 2022-11-21T11:27:17Z

pmp_iv/models/eda.py

+
+    def by_date_property(self, mes, dia, propiedad):
+        data_by_property = []
+        for i in self.valores:


Debes tratar de evitar los bucles for todo lo posible. Usa map o filter o construcciones similares.

JJ · 2022-11-21T11:29:20Z

pmp_iv/models/fecha.py

-from pmp_iv.enums.month import Month
-from pmp_iv.utils.validation import Validation
+from pmp_iv.validaciones.model_validation import ModelValidation
+from pmp_iv.models.day import Day


Una vez más, esto no es una arquitectura MVC. Si no es un modelo de datos, no lo metas en un subdirectorio models. De hecho, no lo metas en ningún subdirectorio, como te he dicho antes.

JJ · 2022-11-21T11:33:17Z

tasks.py


+@task
+def check(c):
+    run("poetry check")


Esto ya te lo dije: poetry check no comprueba la sintaxis.
Esto no sé como te lo dejé pasar en #19 . Cupongo que lo habrás añadido ahora porque te dio error, porque borraste el que tenías originalmente.

for #26

MauronMP · 2022-11-21T13:11:40Z

Buenas @JJ, con inv check ya comprueba la sintaxis, el constructor de la correlación obtiene el cálculo directamente y uso map en la clase EDA, la verdad que está muy bien los cambios que me ha dicho, el test ha pasado de estar 10 segundos a 0.04s. Solicito revisión

JJ

👍

closes #21, closes #22, closes #24, closes #25, closes #26 closes #27, closes #28, closes #29, close #30,closes #31, closes #32

MauronMP added 6 commits November 7, 2022 22:49

test(.toml) && docs(Assertion && Test .md): Elección de cada uno y añ…

24594f9

…adida pytest-tap en .toml

test(.toml && tasks.py) Incluido orden jupyter notebook y añadido lib…

b1385fe

…rerías para jupyter

build(data/ && notebooks/) directorios para una buenas estructura de …

4d4faf1

…proyectos en jupyer

👷 ✨ feat(notebooks/): Clase Datos, lee csv y ejecuta unas funciones. …

3a9433b

…Añadidas dependencias con poetry

👷 ✨ feat(notebooks/): Clase Datos, lee csv y ejecuta unas funciones. …

27a4617

…Añadidas dependencias con poetry

✅ ✔️ 🚨 (tests/*) Creado test para Jupyter y pmp_iv, test funcionando #21

9611d1a

, #22

MauronMP added 🙋 user-stories Etiqueta de historias de usuarios Objetivo_6 🦜 labels Nov 9, 2022

MauronMP added this to the [M1] Transformación de los datos por medio de algoritmos para obtener un perímetro de riesgo de incendio. milestone Nov 9, 2022

MauronMP added 3 commits November 10, 2022 13:33

Bugfix (iv.yaml): Salto linea

ac73844

🚨 (tasks.py) Añadido check

a47aa13

📚 Docs(README.md) Añadido invoke check y test hechos

e04a592

github-actions bot mentioned this pull request Nov 10, 2022

[IV-22-23] MauronMP Objetivo_4 JJ/IV-22-23#170

Merged

3 tasks

JJ suggested changes Nov 10, 2022

View reviewed changes

MauronMP added 5 commits November 10, 2022 15:55

🔧 ✅ (tests/) Ejecutan .py, notebooks eliminados #21, #22

ab588f7

🎨 📚 Reestructuración ficheros proyectos #21, #22

a579ed6

🎨 📚 Reestructuración ficheros proyectos #21, #22

653b0f6

🎨 style(pmp_iv/) Quitado comentarios y generación imagenes correcta

959e196

📚 (docs/img/) Imagenes repetidas

e338bda

MauronMP requested a review from JJ November 10, 2022 15:38

📚 ➖ 🚨 (tasks.py) Remove --tap-stream

11b5aaf

MauronMP requested review from JJ and removed request for JJ November 11, 2022 11:56

JJ suggested changes Nov 11, 2022

View reviewed changes

MauronMP added 2 commits November 14, 2022 14:01

🚨 refactor(Datos.py && test_Datos.py) removed pandas, using csv module …

49817e4

…#21, #22, #24, #7

🎨 style(Invoke_test.png) Updated test image demostration for #24

dd29313

JJ suggested changes Nov 14, 2022

View reviewed changes

✨ feat(regression_algorithm.py) Tipo enumerado, nombre algoritmos usa…

83b4e8a

…dos #29

JJ suggested changes Nov 16, 2022

View reviewed changes

MauronMP modified the milestones: [M2] Transformación de los datos por medio de algoritmos para obtener un perímetro de riesgo de incendio., [M1] Obtención de la correlación a partir de los datos sesgados del csv. Nov 16, 2022

MauronMP added 5 commits November 16, 2022 14:24

✅ test(Invoke_test.png): Tests passed #31

aa1d0a6

🎨 style (eda.py): Typo error #25

894b416

✅ test(test_pmp_iv.py): Comrpueba correlacion #26 #31

e22797d

👷 build(regression && model.py): Delete, no son necesarios para #26

0efb5eb

✨ feat(correlacion_area.py): Obtención correlación dato-area #26

3330d68

MauronMP requested a review from JJ November 16, 2022 13:35

🚨 refactor(pyproject.toml) removed scikit-learn #31

5527a51

MauronMP removed the request for review from JJ November 17, 2022 10:51

JJ suggested changes Nov 18, 2022

View reviewed changes

MauronMP added 5 commits November 21, 2022 11:58

💚 ci(pmp_iv/) Reestructuración directorio #25, #26

c868000

✅ test(test_pmp_iv.py): Using fixtures, arrange, act

54e9cd6

and assert #31

💚 ci(pmp_iv/): Update imports #25, #26 and #31

f0894d7

♻️ refactor(eda.py): Set to float values #25

92e90f2

♻️ refactor(correlacion_area.py): Constructor receive eda values

38206e8

for #26

JJ suggested changes Nov 21, 2022

View reviewed changes

MauronMP added 6 commits November 21, 2022 13:57

💚 ci(pmp_iv/models/*) Delete folder #25, #26

06a1081

✅ test(tasks.py): Check syntax pmp_iv/ #32

e2423ff

✅ test(tests_pmp:iv.py): Removed syntax test #31

7b6fa23

💚 ci(pmp_iv/): Restructuring files #25, #26

4a389b9

♻️ refactor(eda.py) Using map instead of loop for #25

21e0bf5

♻️ refactor(correlacion_area.py): Constructor makes operations

7e330c6

for #26

JJ approved these changes Nov 21, 2022

View reviewed changes

📝 docs(README.md) Update test image and docs links.

8d31ded

closes #21, closes #22, closes #24, closes #25, closes #26 closes #27, closes #28, closes #29, close #30,closes #31, closes #32

MauronMP merged commit 20ba587 into main Nov 21, 2022

		@@ -0,0 +1,47 @@
		from sklearn.preprocessing import StandardScaler

[IV-22-23] Objetivo 4 PMP_IV #23

[IV-22-23] Objetivo 4 PMP_IV #23

Conversation

MauronMP commented Nov 9, 2022 • edited Loading

JJ left a comment

Choose a reason for hiding this comment

MauronMP commented Nov 10, 2022 • edited Loading

JJ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MauronMP commented Nov 14, 2022

JJ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JJ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MauronMP commented Nov 17, 2022

JJ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MauronMP commented Nov 21, 2022

JJ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MauronMP commented Nov 21, 2022

JJ left a comment

Choose a reason for hiding this comment

MauronMP commented Nov 9, 2022 •

edited

Loading

MauronMP commented Nov 10, 2022 •

edited

Loading