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

aam.fitter, clm.fitter, sdm.trainer packages #432

Merged
merged 5 commits into from Sep 6, 2014

Conversation

Projects
None yet
2 participants
@jabooth
Member

jabooth commented Sep 5, 2014

Follows #431.

  1. Moves AAM and subclasses from aam.builder to aam.base
  2. Moves AAMFitter and subclasses from aam.base to aam.fitter
  3. Moves CLM and subclasses from clm.builder to clm.base
  4. Moves CLMFitter and subclasses from clm.base to clm.fitter
  5. Moves SDTrainer and subclasses from sdm.base to sdm.trainer
  6. Moves SDFitter and subclasses from sdm.base to sdm.fitter

@jabooth jabooth added the in progress label Sep 5, 2014

@jalabort

This comment has been minimized.

Member

jalabort commented Sep 5, 2014

This is a bit confusing in my opinion... Since the package is essentially called fit

@jabooth

This comment has been minimized.

Member

jabooth commented Sep 5, 2014

I agree from that perspective is weird, but I feel this namespacing is clearer once you are within one of the 3 deformable model packages (AAM, CLM, SDM).

Thought experiment:

  1. Do you think that all the code for a given deformable model (e.g. anything related to AAMs) should be kept in a single package? This is of course how we currently have it, but the alternative would be a grouping by functionality (e.g a builder package contains builders for all 3. etc etc)
  2. If you are happy with grouping by model type, how would you split up the namespace for each model? I personally would go with the arrangement in this PR, but happy to hear alternatives.
  3. Given the above, why is this package called fitmultilevel? It doesn't just contain fitting as you rightly said, it also contains model construction and the definitions of the models themselves. We've already said its maybe not the most obvious name to people who aren't familiar with Menpo (take for instance the decision to call the folder of notebooks for this part of the code 'Deformable Models'). Maybe we should just rename the package menpo.deformablemodel, then everything is clearer? We can still have MultiLevelFitter etc as the naming is perfectly logical, but most end users of Menpo never see this so I don't see any cause for confusion.
@jalabort

This comment has been minimized.

Member

jalabort commented Sep 5, 2014

This second proposal is not that controversial... I think it might be better.

The fit-fitmultilevel structure was almost self imposed by the way Menlo was growing when we decided to name the packages that way. Basically because of the difficult relationship between the aam package and the lucas kanade package.

The results was we currently have in menpo. This structure is weird in my opinion cause I have an aam package for which all the algorithms are in another package (fit) but we thought it made sense at the time... The same holds for clm and sdm.

I think that could be better designed, but at the time I really didn't know how to do it. What you're proposing seems reasonable but we ought to decides what do we do with the fit package. I personally would put the clm algorithms and regressors were they belong, the clm and sdm packages and I'm just not sure what to do with Lucas kanade because of its more complex relationship with aams

@jabooth

This comment has been minimized.

Member

jabooth commented Sep 5, 2014

I actually like your proposal for menpo.fit items becoming subpackages of the deformable models. Formalising the whole thing:

  1. All code that is specific to deformable model construction and fitting is contained in the menpo.deformablemodel package. Some routines that have utility out of this area may be extracted and moved to new homes in menpo core (as I did with mean_pointcloud in #431).
  2. Inside this package are sub-packages for AAMs, CLMs and SDMs named aam, clm and sdm.
  3. Inside each package:
    1. Models themselves go in base (e.g. AAM lives at menpo.deformablemodel.aam.base.AAM)
    2. Fitters go in fitters
    3. Builders in builders / trainers in trainers
  4. Low level fitting routines are just stored with the deformable model they are required for.
  5. Abstract classes and interfaces for the whole package are stored in deformablemodel.base

Pictorially:

deformablemodel
|- base
|  |-builder  # contains generically useful deformable model construction functions 
|  |-fitter   # contains Fitter, MultilevelFitter etc
|  |-fittingresult  # FittingResult, MultiLevelFittingResult etc
|
|- clm
|  |-base
|  |-builder
|  |-fitter
|  |-gradientdescent
|     |-...
|
|- sdm
|  |-trainer
|  |-fitter
|  |-regression
|     |-...
|
|- aam
   |-base
   |-builder
   |-fitter
   |-lucaskanade
       |- ...

I know that this breaks the perception that users should be able to just grab something from menpo.fit.lucaskanade and use it, but it does make the whole package seem much more cohesive - and of course there is nothing to stop people importing LK items from menpo.deformablemodel.aam.lucaskanade. I personally think the trade-off is well worth it.

@jabooth jabooth referenced this pull request Sep 6, 2014

Merged

tidy classifiers #433

@jabooth

This comment has been minimized.

Member

jabooth commented Sep 6, 2014

@jalabort no need to rush on the larger issue I've raised here (renaming fitmultilevel, what to do with fit), but given we are probably going in that direction are you happy with the changes this PR makes? Unfortunately my further work on the package is on top of this PR so I would like to bring it in if possible - in a future PR we can rename fitmultilevel and deal with the fit package. Happy with that?

@jalabort

This comment has been minimized.

Member

jalabort commented Sep 6, 2014

Yeah happy

@jalabort

This comment has been minimized.

Member

jalabort commented Sep 6, 2014

+1

jabooth added a commit that referenced this pull request Sep 6, 2014

Merge pull request #432 from jabooth/fitterpy
aam.fitter, clm.fitter, sdm.trainer packages

@jabooth jabooth merged commit 6a2aed9 into menpo:master Sep 6, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@jabooth jabooth removed the in progress label Sep 6, 2014

@jabooth jabooth deleted the jabooth:fitterpy branch Sep 6, 2014

@jalabort

This comment has been minimized.

Member

jalabort commented Sep 6, 2014

I believe that's exactly how it should be, yes!

In my opinion it is much clearer this way.

Thanks,

Joan

On 5 Sep 2014 22:32, James Booth notifications@github.com wrote:

I actually like your proposal for menpo.fit items becoming subpackages of the deformable models. Formalising the whole thing:

  1. All code that is specific to deformable model construction and fitting is contained in the menpo.deformablemodel package. Some routines that have utility out of this area may be extracted and moved to new homes in menpo core (as I did with mean_pointcloud in #431#431).
  2. Inside this package are sub-packages for AAMs, CLMs and SDMs named aam, clm and sdm.
  3. Inside each package:
  • Models themselves go in base (e.g. AAM lives at menpo.deformablemodel.aam.base.AAM)
  • Fitters go in fitters
  • Builders in builders / trainers in trainers
  1. Low level fitting routines are just stored with the deformable model they are required for.
  2. Abstract classes and interfaces for the whole package are stored in deformablemodel.base

Pictorially:

deformablemodel
|- base
| |-builder # contains generically useful deformable model construction functions
| |-fitter # contains Fitter, MultilevelFitter etc
| |-fittingresult # FittingResult, MultiLevelFittingResult etc
|
|- clm
| |-base
| |-builder
| |-fitter
| |-gradientdescent
| |-...
|
|- sdm
| |-trainer
| |-fitter
| |-regression
| |-...
|
|- aam
|-base
|-builder
|-fitter
|-lucaskanade
|- ...

I know that this breaks the perception that users should be able to just grab something from menpo.fit.lucaskanade and use it, but it does make the whole package seem much more cohesive - and of course there is nothing to stop people importing LK items from menpo.deformablemodel.aam.lucaskanade. I personally think the trade-off is well worth it.


Reply to this email directly or view it on GitHubhttps://github.com//pull/432#issuecomment-54684299.

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