New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new PCAVectorModel class, refactor model package #645

Merged
merged 26 commits into from Nov 23, 2015

Conversation

Projects
None yet
3 participants
@patricksnape
Contributor

patricksnape commented Nov 23, 2015

This adds a version of PCAModel that works on vectors (numpy arrays). PCAModel continues to work on menpo instances, but now we have PCAVectorModel, which implements most of PCA and operates on numpy arrays.

I also refactored the 'instance' models to be called 'vectorizable' instead.

Two other changes:

  • Add the normalized_weights flag to PCA for creating instances with weights normalized by the standard deviation (1.0 is one standard deviation)
  • Make sure that the scatter matrix properly represents the complex conjugate.

patricksnape and others added some commits Nov 17, 2015

Add the GMRF model
This is a squashed commit that adds the GMRF model as @nontas
originally defined it. Here I am extracting out the parts
that @nontas and I had merged when we added my PCA changes.
This includes mostly GMRF stuff (pcacov, and graph methods)
but it also includes the gaussian ellipses changes.
Ensure projection (for project out) is correct
Should use the project_vectors method to ensure that we calculate
the weights correctly (for example, a mean linear model will
need to subtract the mean first). However, projecting out
does not involve re-adding the mean so the dot product
should be correct.
Targetable should return self
This makes it much easier to chain set_target().target calls
which is handy for menpofit
Add a cca method
Calculates CCA between two matrices.
Ensure project-out is correct (PCA)
Make sure we use the correct methods for turning weights into
instances. In particular, make sure that projecting out doesn't
re-add the mean for PCA
Starting to refactor the pca model
Refactoring into two classes, one for 'instances' and one
for numpy arrays
Refactor PCAModel to PCAVectorModel
Moves the _vector methods onto the instance backed model,
which is now vectorizablebackedmodel. This is a partial commit
as I leave to go to Stansted!
fix init_from components to set the template
The init_from_components of PCAModel was not setting the template.
This is fixed now by initialising VectorizableBackedModel with
the mean isntance.
Update documentation on doc_inherit
Also, hide previously public methods
Use allclose rather than == 0
For 0 check in PCAModel variance
Refactor to self_model
Make it more clear that 'model' mimicking self
@jabooth

This comment has been minimized.

Member

jabooth commented Nov 23, 2015

You know there's GRMF stuff in here too? Docs etc

Remove GMRF
Also, fix up documentation
@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Nov 23, 2015

@jabooth Done.

@jabooth

This comment has been minimized.

Member

jabooth commented Nov 23, 2015

@patricksnape still see

  • docs/source/api/menpo/shape/chain_graph.rst etc
  • docs/source/api/menpo/visualize/plot_gaussian_ellipses.rst etc
  • menpo/landmark/labels/human/face.py etc
  • menpo/visualize/base.py etc

Are they supposed to be in here? Seem GRMF related..?

patricksnape added some commits Nov 23, 2015

Undo graph updates
This may leave things a bit messy but this way @nontas gets to
bring all his changes in cleanly
# print('\nNo .copy() (shallow copied):')
# for k, v in non_copies.items():
# print(' {:15}| {}'.format(k, ', '.join(v)))

This comment has been minimized.

@jabooth

jabooth Nov 23, 2015

Member

Think this might be useful to leave in? Or if you feel strongly about removing it get rid of the four lines referenced in Copyable too.

No problem with you removing it, I'll just make a little gist with it for future reference.

This comment has been minimized.

@patricksnape

patricksnape Nov 23, 2015

Contributor

Just feels weird having a load of commented out lines of code? Since it's in the history anyway?

This comment has been minimized.

@jabooth

jabooth Nov 23, 2015

Member

No I agree, like I said just remove the other commented out lines and I'll make a gist

This comment has been minimized.

@jabooth

jabooth Nov 23, 2015

Member

I've added an entry about this in the wiki.

"""
self._target_setter_with_verification(new_target) # trigger the update
self._sync_state_from_target() # and a sync
return self

This comment has been minimized.

@jabooth

jabooth Nov 23, 2015

Member

This seems like quite a large change in behavior, why add it?

On a mutable setter I would argue that it is clearer to not return self, that way there is no sense for the programmer that you might be using a immutable setting API.

This comment has been minimized.

@patricksnape

patricksnape Nov 23, 2015

Contributor

Because otherwise writing all the AAM code is a huge annoyance because every time we set_target we need another line just to grab the target we just set. It was just tons cleaner to be able to do set_target(blah).target

This comment has been minimized.

@jabooth

jabooth Nov 23, 2015

Member

Fair enough, but this behavior is inconsistent with the rest of our mutating API's (from the top of my head compose_{before,after}_inplace, from_vector_inplace, apply_inplace) Saying that, I'm sure some classes do return self from those methods. It's an inconsistency I've been meaning to resolve for a while, guess we just need to decide which way to go.

@@ -276,6 +276,70 @@ def face_ibug_68_to_face_ibug_49(pcloud):
return new_pcloud, mapping


@labeller_func(group_label='face_ibug_49')
def face_ibug_49_to_face_ibug_49(pcloud):

This comment has been minimized.

@jabooth

jabooth Nov 23, 2015

Member

just document this addition in the PR description if you want to add it here, so it's discoverable when we come to write up the changelog

@@ -96,10 +166,10 @@ def pca(X, centre=True, inplace=False, eps=1e-10):
if d < n:
# compute covariance matrix
# C (covariance): d x d
C = np.dot(X.T, X) / (n - 1)
C = np.dot(X.conj().T, X) / (n - 1)

This comment has been minimized.

@jabooth

jabooth Nov 23, 2015

Member

these are fixes for complex PCA right? Maybe add them to the PR description? I take it we don't break behavior for existing matrices with these changes?

This comment has been minimized.

@patricksnape

patricksnape Nov 23, 2015

Contributor

No we just discussed it and though that this was actually the correct behaviour anyway. But sure.

from .linalg import dot_inplace_right


def cca(X, Y, inplace=False):

This comment has been minimized.

@jabooth

jabooth Nov 23, 2015

Member

This is added and not used anywhere else in this PR - intentional? I would rather it was a separate PR if possible. Is it related to GRMF?

This comment has been minimized.

@patricksnape

patricksnape Nov 23, 2015

Contributor

No it was just something I needed for my menpofit branch

if self.n_active_components == self.n_components:
noise_variance = 0.0
if self._trimmed_eigenvalues.size is not 0:
noise_variance += self._trimmed_eigenvalues.mean()

This comment has been minimized.

@jabooth

jabooth Nov 23, 2015

Member

better as

if (self.n_active_components == self.n_components and 
    self._trimmed_eigenvalues.size != 0):

right?

To be clear:

  1. nested if's are redundant
  2. the is equality check with 0 is dangerous

patricksnape added some commits Nov 23, 2015

Missed import
Removing 49->49 labels
Fix incorrect is not 0
Also, nested if statements is ugly and pointless.
Remove cca
Will make into a new PR
No longer return self from set_target
This is to keep the API consistent though it will break a lot
of my code in menpofit. It makes updating a target and getting
the new target a lot uglier but setters generally don't return
anything so it's better to behave predictably.
@jabooth logic wasn't quite correct
We did need a nested loop but the inlining of the 0 was better.
Not using is with numbers still stands, however
@nontas

This comment has been minimized.

Member

nontas commented Nov 23, 2015

That looks good +1

@jabooth

This comment has been minimized.

Member

jabooth commented Nov 23, 2015

@patricksnape nice work! +1

jabooth added a commit that referenced this pull request Nov 23, 2015

Merge pull request #645 from patricksnape/pcamodel
Add new PCAVectorModel class, refactor model package

@jabooth jabooth merged commit 009625c into menpo:master Nov 23, 2015

3 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jabooth jabooth removed the in progress label Nov 23, 2015

@jabooth jabooth deleted the patricksnape:pcamodel branch Nov 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment