byuflowlab / VortexLattice.jl

A Comprehensive Julia implementation of the Vortex Lattice Method
MIT License
48 stars 11 forks source link

Restructuring Code/Performance Updates #5

Closed taylormcd closed 3 years ago

taylormcd commented 3 years ago

Changes:

I have not changed the theoretical foundation yet.

Future work:

andrewning commented 3 years ago

I'm going to merge, but just had a couple questions/comments.

Why are these lines needed (and others like it): Base.eltype(::Panel{TF}) where TF = TF

How did you determine when inlining was needed? Does it really help much?

I like putting the stability derivatives separate with AD. The downside is that if you want derivatives of the stability derivatives you'll have to use nested AD, which can be slow (though I haven't profiled it in this case).

taylormcd commented 3 years ago

Those eltype definition lines are in order to be able to extract type information from the structs. It's useful when combined with promote_type to find the correct type of float to use in functions. I suppose I could have used a custom function instead of overloading eltype, but I think overloading eltype is more standard.

taylormcd commented 3 years ago

I put inline on all the internal functions of the performance critical functions. It helps with performance when using static arrays.

taylormcd commented 3 years ago

With regards to AD, you were basically implementing your own AD already, so I wouldn't expect nested AD to be any slower than when using the previous version, especially since the version of AD in ForwardDiff is more optimized for performance.

andrewning commented 3 years ago

I've never needed promote_type in order to make my functions AD compatible. You must have a different approach from what I normally do. I wasn't implementing AD, just symbolic derivatives, but you're probably right that the performance won't be much different.