-
Notifications
You must be signed in to change notification settings - Fork 70
Conversation
Cessna310.py is added unfinished, Lat_Aero_Coefficients, gets_forces and gets moments are missing.
Cessna310.py is completed but it needs to be reviewed again Cessnat310.py provide forces and moments
Le pongo la etiqueta de revisión. |
|
||
return moments | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trim extra whitespace
He revisado el estilo, básicamente he puesto algunos comentarios sobre las cadenas de documentación y el resto me parece que está bien. Faltan tests por añadir, eso sí. |
Lat_coef_matrix = Lat_Aero_Coefficients() | ||
|
||
CLfull = np.dot(Long_coef_matrix[0,:],long_control) | ||
CDfull = np.dot(Long_coef_matrix[1,:],long_control) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A mí esta forma de hacer los cálculos de los coeficientes me parece un poco complicada: creo que no se gana mucho en rendimiento por 'vectorizar' así el código (no son grandes arrays) y sin embargo es bastante más difícil ver lo que hace la fórmula. Yo tendería hacia poner las fórmulas "tal cual". ¿Qué os parece?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perdona, el comentario iba aquí= A mi personalmente no me gusta nada tener una función con 9 salidas, me parece dificil de trabajar y de testear. Pero si pensais que es una mejor forma lo corrijo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quizá no esté mal que la función devuelva los coeficientes empaquetados de alguna forma (esta no tiene por qué estar mal, pero tenemos que pensar en ello quizá un diccionario o alguna otra estructura sea más adecuada). Pronto esos coeficientes estarán metidos como atributos en una clase, así que ya habrá tiempo de cambiarlo si es necesario. A lo que me refiero en concreto es a calcular el CLfull y el CDfull con ese producto escalar.
A lo mejor es más claro desempaquetar al estilo:
CL0, CLa, CLde, CLdih = Long_Aero_Coefficients()
y poner el CLfull como:
CLfull = CL0 + CLde * delta_e ...
Así de un vistazo queda claro que modelo de fuerzas estamos usando.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vale, lo cambio
|
||
|
||
|
||
def test_q(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martosc aquí tienes el test de la presión dinámica y en el del avión la función por si quieres trabajar sobre ellas directamente.
Por mi parte esta guay! te he comentado simplemente lo de geometric_data por el mismo motivo que los coeficientes. Por cierto, creo que ya lo habíamos comentado, pero no tiene motor, no? Pasa el pep8 y mira el archivo en Spyder con Pylint para el tema del estilo (sobre todo los saltos de línea) y en cuento tenga motor, por mi parte está terminado. No tenemos por qué complicarnos de momento con el modelo de motor, nos vale que le entre la posición de la palanca (entre 0 y 1) y que de un porcentaje del empuje máximo. Queda pendiente discutir qué pasa con los límites de mando, no puedes tener un delta_e de 90º por ejemplo. Pero esto lo podemos dejar pendiente. Ah! y el nombre del archivo en minúsculas mejor, para seguir el pep8 https://www.python.org/dev/peps/pep-0008/#package-and-module-names |
Ahora modifico lo de geometric_data. La parte del motor yo pensaba que no iba incluida en este archivo porque el issue se llamaba fuerzas aerodinámicas. |
@AlexS12 y otra cosa, no tengo el empuje máximo del motor entre los datos |
AIRCRAFT DYNAMICS From modelling to simulation (Marcello R. Napolitano) | ||
page 589 | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me sobra este espacio en blanco aquí también.
Cerrando y abriendo para que salten los tests en Travis CI. |
Le cambio la etiqueta porque @olrosales está todavía trabajando en ello. |
Para revisar Óscar? |
Me he perdido con el estado de esta rama. Qué es lo que está in progress en concreto? |
Sorry! Veo que está en Needs Review... Me he confundido esta mañana. @olrosales, si pasas el pep8 te da un montón de espacios en blanco al final de línea. (Esto puede ser interesante para todos @AeroPython/pyfme) Por lo demás, veo que está bastante bien. Los nombres son los que acordamos si no me equivoco, no? Salvo un par de tonterías que te marco ahora, todo ok por mi parte 👍. En cuanto cambies esto, fusionamos el pull request y ya tenemos avión de pruebas! 😄 |
|
||
Returns | ||
---- | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Esta línea en blanco es prescindible, aunque creo que no da problemas al generar la documentación. Ocurre lo mismo en otras funciones.
Te he marcado algunas, básicamente es copiar lo que dice el pep8. Configúrate Spyder como te he puesto arriba e intenta quitar triangulitos de warning y a funcionar! |
@AeroPython | ||
""" | ||
|
||
## Aircraft = Cessna 310 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
¿Lo movemos al docstring del module? (Donde está la fecha, el autor...)
Ya están hechos todos los cambios, así que vamos a integrar esto. ¡Gracias @olrosales! 🎉 |
🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 |
Cessna 310 was added as Cessna310.py.