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

Add filtered operators and initializer #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add filtered operators and initializer #33

wants to merge 1 commit into from

Conversation

lehnerpat
Copy link

These three classes are variants of the standard initializer (eoInit) and the main operators (eoMonOp and eoQuadOp) which filter the output individuals by a simple mechanism:

  • create an instance C, which takes an object of its superclass to do the actual work
  • add an arbitrary number of filter functions with the signature bool function(EOT individual)
  • when operator() is called on C,
    • the operators will call their underlying operator to create one or two new individuals, and then apply all filter functions; if any of them returns false ( "not accepted" ), the created individual(s) is/are rejected and operator() will return false ( "individual(s) unchanged" )
    • the initializer will call its underlying initializer to create one individual, and then apply all filter functions; if any of them returns false ( "not accepted" ), it will use the underlying initializer to create a new individual; this is repeated until the individual is accepted

Some points to consider and discuss:

  • would you like to have tests for these classes? if so, do you have any guidelines for unit tests? (I have no experience with tests myself, but i'm willing to actively contribute)
  • I will add some Doxygen comments soon
  • shall eoInitFilter be renamed to eoFilterInit (so all classes follow a common scheme) ?
  • do you think an analogous class for eoBinOp should be created?
  • the files are currently carrying a GPLv3 license notice, but I saw that most other files have GPLv2 instead; if you want to keep the whole project under v2, I can change the license for the files.

@nojhan nojhan self-assigned this Jun 30, 2019
@nojhan
Copy link
Owner

nojhan commented Dec 6, 2019

Thanks for this feature.

To answer your questions:

  1. yes, tests should be added in the /test/ directory. It's just a matter of adding a t-<class>.cpp binary that use assert to check the results. Then adding the name in the cmake configuration in the same directory. Just look at the existing tests.
  2. Yes, please, you can start by using the above message as a comment.
  3. I prefer to name from more general to more specific, so I would favor eoInitFilter.
  4. It would be nice to have, yes.
  5. The license of the EO module should be LGPLv2, or I will not merge it because mixing license is a problem (yes we should clean the whole repository indeed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants