Skip to content
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

Generic functor holder and constant image from functor. #1332

Merged
merged 35 commits into from
Jan 26, 2019

Conversation

rolanddenis
Copy link
Member

@rolanddenis rolanddenis commented Jul 10, 2018

PR Description

This PR adds:

  • FunctorHolder: hold any callable object (function, functor, lambda, ...) as a C(Unary)Functor model without relying on std::function
  • FunctorConstImage: Transform a point-dependant functor into a constant image.
  • PointFunctorHolder: hold any callable object, that accept a point, as a CPointFunctor model
  • a module page about functions, functors and lambdas in DGtal.

Checklist

  • Unit-test of your feature with Catch.
  • Doxygen documentation of the code completed (classes, methods, types, members...)
  • Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
  • All continuous integration tests pass (Travis & appveyor)
  • Example file
  • Module page about passing function as parameter

@dcoeurjo
Copy link
Member

Thx Roland for the PR !

@dcoeurjo dcoeurjo added the Base label Jul 10, 2018
@dcoeurjo dcoeurjo added this to the 1.0 milestone Jul 10, 2018
@rolanddenis
Copy link
Member Author

I'm writing a huge documentation about FunctorHolder, its usage and how to make a custom CUnaryFunctor model that rely on it ... maybe it would be better to put that doc in a dedicated page, like "Using functions, functors and lambdas in DGtal with FunctorHolder" ?

@dcoeurjo
Copy link
Member

Definitely agree... please do so

@dcoeurjo
Copy link
Member

BTW I don't think we have any doc on all functions in BasicPointFunctors, BasicPointPredicates... beside the functors namespace

@rolanddenis
Copy link
Member Author

So we should choose a more generic name, like "Using functions, functors and lambdas in DGtal", starting with a list of available functors and ending with the current documentation of FunctorHolder about how creating its own DGtal compatible functors ?

@dcoeurjo
Copy link
Member

Yep

@dcoeurjo
Copy link
Member

thx

@rolanddenis
Copy link
Member Author

Documentation moved (without the functors list) :octocat:

@JacquesOlivierLachaud
Copy link
Member

Seems perfect to me. The doc module is very complete and very clear. Congrats !
I wait for David's comments before merging.

ChangeLog.md Outdated Show resolved Hide resolved
ChangeLog.md Outdated Show resolved Hide resolved
@dcoeurjo
Copy link
Member

dcoeurjo commented Nov 4, 2018

(my comments on the Changleog were due to a pending review.. I'll continue my review)

Copy link
Member

@dcoeurjo dcoeurjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments on the doc (I've tried the new github suggestion for the typos..)

src/DGtal/base/doc/moduleFunctors.dox Outdated Show resolved Hide resolved
src/DGtal/base/doc/moduleFunctors.dox Show resolved Hide resolved
src/DGtal/base/doc/moduleFunctors.dox Outdated Show resolved Hide resolved
src/DGtal/base/doc/moduleFunctors.dox Outdated Show resolved Hide resolved
src/DGtal/base/doc/moduleFunctors.dox Outdated Show resolved Hide resolved
src/DGtal/base/doc/moduleFunctors.dox Outdated Show resolved Hide resolved
src/DGtal/base/doc/moduleFunctors.dox Outdated Show resolved Hide resolved
@rolanddenis
Copy link
Member Author

Before merging, this PR may need to change its naming convention:

  • the CUnaryFunctor model is FunctorHolder (but it is not only for unary functors) with construction helper called holdFunctor,
  • the CPointFunctor model is PointFunctorHolder with construction helper called makePointFunctorHolder,
  • the CConstImage model is FunctorConstImage with construction helper called makeFunctorConstImage.

What do you think ?

@JacquesOlivierLachaud
Copy link
Member

This PR looks ready to be merged for me. @dcoeurjo Are you ok ?

@dcoeurjo
Copy link
Member

dcoeurjo commented Dec 1, 2018

Fine for me but @rolanddenis has pending questions I think

@dcoeurjo
Copy link
Member

dcoeurjo commented Jan 5, 2019

@rolanddenis can you please update this branch w.R.t. the master ?

@rolanddenis
Copy link
Member Author

Done :octocat:

@rolanddenis
Copy link
Member Author

About my previous question, I was thinking about a naming like that:

  • FunctorHolder and holdFunctor
  • maybe an alias to be consistent: UnaryFunctorHolder and holdUnaryFunctor
  • PointFunctorHolder and holdPointFunctor
  • ConstImageFunctor (and/or Holder ?) (instead of FunctorConstImage) and holdConstImageFunctor

Class name = concept name + (Functor) Holder
Construction helper = hold + class name without Holder
@rolanddenis
Copy link
Member Author

I have modified the naming scheme accordingly to my last comment (except for the UnaryFunctorHolder), so it should be all good!

@dcoeurjo
Copy link
Member

Thanks @rolanddenis . Merging

@dcoeurjo dcoeurjo merged commit bf5266a into DGtal-team:master Jan 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants