champsproject / ldds

Python package for computing and visualizing Lagrangian Descriptors in Dynamical Systems
BSD 3-Clause "New" or "Revised" License
18 stars 2 forks source link

avoid transposing arrays in vector fields #15

Closed vkrajnak closed 4 years ago

broncio123 commented 4 years ago

Can you provide an example of where this has been done?

vkrajnak commented 4 years ago

https://github.com/broncio123/lds/blob/b8b59619b417ef8ba437a263a6493061ddeaa414/pylds/vector_fields.py#L32-L35

I'll try to find a way around, it makes the code hard to read

vkrajnak commented 4 years ago

solve_ivp accepts matrices

broncio123 commented 4 years ago

https://github.com/broncio123/lds/blob/b8b59619b417ef8ba437a263a6493061ddeaa414/pylds/vector_fields.py#L32-L35

I'll try to find a way around, it makes the code hard to read

Hi @vkrajnak and @VikJGG

To improve readability line breaks can be implemented, as suggested in PEP8 guidelines, here

So, the above code can be rewritten as

 v = np.array([
               omega * y, 
             - omega * x
                     ]).T 

Although not mentioned in PEP8, an alternative approach might be defining array entries as variables, combined an explicit transposition operator, for instance

v_x = omega * y
v_y = - omega * x
v =  np.transpose( np.array([v_x, v_y]) )

What do you guys think?

As for matrix notation, I haven't employed ir myself much, but if that improves readability we can discuss it. Mad love, b

broncio123 commented 4 years ago

solve_ivp accepts matrices

Raise separate issue for this, please. @vkrajnak

vkrajnak commented 4 years ago

On the contrary, this is the perfect solution to the issue

vkrajnak commented 4 years ago

The function check_arguments called in the class OdeSolver requires arguments to be 1-dimensional. Making it work would require digging deep into the integration routine. For now, doing away with some of the transposing will have to do.

vkrajnak commented 4 years ago

Pull request #28