search

menpo / cyvlfeat

A thin Cython wrapper around select areas of vlfeat
BSD 2-Clause "Simplified" License
109 stars 72 forks source link

update document to reflect the array ordering change and make it consistent in the pkg #54

Open hubutui opened 4 years ago

hubutui commented 4 years ago

Correct me if I get it wrong. Accoring to https://github.com/menpo/cyvlfeat/commit/c8e496f3a72c6ddb313cc0c029419124c0b8fc7c, now the array is in shape of [n_samples, n_features], just like scikit-learn or other python packages. So, https://github.com/menpo/cyvlfeat/blob/102ba95c3f80fc913d7cabaa8e145fd6727f7566/cyvlfeat/fisher/fisher.py#L16-L17 I think you mean

One row per data vector (e.g. a SIFT descriptor)

And there are some inconsistent usage, say vlad, when I update the commits from it use different array ordering. Now we have two different array ordering, which will make users confused. We need to make it consistent.

patricksnape commented 4 years ago

As you can see from #20 there was a lot of discussion about which ordering was the "right" ordering. And for the fisher module it was agreed in that PR that the [n_samples, n_features] was appropriate for the underlying call to vlfeat.

Obviously it would be best to standardize this and test that the underlying modules actually act correctly - but this is beyond my personal scope as I don't use any of the modules recently contributed. Again - happy to try and generally review and cut releases but don't have the scope to actually make this wrapper feature complete.

patricksnape commented 4 years ago

First part addressed in #59