search

ajdawson / windspharm

A Python library for spherical harmonic computations on vector winds.
http://ajdawson.github.io/windspharm
MIT License
82 stars 38 forks source link

Consider making truncate and gradient functions rather than VectorWind methods #73

Closed shoyer closed 8 years ago

shoyer commented 8 years ago

This is one part of the windspharm API that doesn't make sense to me.

As far as I can tell, the direct arguments to truncate and gradient contain all the information necessary to calculate them. Indeed, based on my tests, I get identical results when I call these methods on a VectorWind instance initialized with u and v fields multiplied by 0.

In general, methods that do not rely on any properties of the attached object should simply be functions. In this case, this would have the additional advantage of letting users use these methods even if u and v fields are not available.

ajdawson commented 8 years ago

The rationale for this design is that these methods are designed to operate on output of the VectorWind instance, and require shape compatibility with them.

In general, methods that do not rely on any properties of the attached object should simply be functions.

I'm aware of this basic principle of object-oriented design, and although the gradient and truncate methods do not use the values of u and v, they do use the spherical harmonic transform kernel created from the properties of the u and v fields. If they were functions we'd have to create a new transforms engine for each quantity, or have the user pass one in.

The use case these methods were created for is when you produce a scalar quantity from the windspharm object then want to compute the gradient, you know the shape is compatible with the spherical harmonic transform engine used to compute it (by definition) so it makes sense to reuse this rather than create a new one. This is demonstrated in the Rossby wave source example where the gradients of vorticity are required.

windspharm was never really meant to be a general purpose wrapper for pyspharm, but a facility for computing properties of the flow (defined by u and v).

shoyer commented 8 years ago

If they were functions we'd have to create a new transforms engine for each quantity, or have the user pass one in.

Okay, this is a fair point that I didn't quite realize. It seems reasonable to keep these on VectorWind, then. A comment in the code to clarify this reason could be a worthwhile note to future contributors.