acroy / Expokit.jl

Julia implementation of EXPOKIT routines
Other
26 stars 11 forks source link

Add padm.jl #14

Closed mforets closed 6 years ago

mforets commented 6 years ago

It seems that @compat is not working properly: the different ways of writing function padm(...) are all valid in v0.6 . Any suggestion?

acroy commented 6 years ago

I am not sure about the @compat issue, but I would remove the type annotations (for A and the function) altogether. The reasons to have type annotations are either to help Julia to do type inference or to have different versions of the same function for different input types. Neither is necessary here. A just has to support a set of operations (norm, full, ...) otherwise a MethodError exception will be thrown (that is basically the idea of duck typing). Specialisation is handled automatically. There should be no difference in performance either.

mforets commented 6 years ago

thank you for the feedback. indeed, i was keeping the type annotation in padm for performance, as hinted in the 2nd item in the Types Declarations docs.

a little benchmark with size(A) = (1000, 1000) and p=0.001 gives me this:

no type annotation type annotation expm(full(A))
129.873 ms 134.829 ms 398.629 ms
Jutho commented 6 years ago

Type annotation can only help for performance when used for a variable inside the function body that would otherwise not be inferred correctly (and thus dynamic / Any); though in most cases you're better of trying to fix the type instability.

In most cases, function arguments are always specialized upon, i.e. a specific function is compiled for every different type of input arguments, and type annotation in function arguments is only useful to restrict that particular method definition to only act on those types.

acroy commented 6 years ago

Great! Can you maybe squash the commits?

mforets commented 6 years ago

yes. let me apply your reviews from today and squash. also, you guys are not writing the real name on julia docstrings, so for consistency i'll remove mine as well.

mforets commented 6 years ago

Done! i don't know what is the problem reported by Travis, but it seems it coudn't install julia 0.6

acroy commented 6 years ago

Thanks! I think the error is unrelated.

mforets commented 6 years ago

thanks for reviewing.