FV-MOEA #1
FV-MOEA #1
Conversation
There was a problem hiding this comment.
Thank you for PR.
What I haven't done here:
- my memory is not fresh, so didn't do actual, thorough review.
- didn't verify if FVMOEA fits with metaalgorithms like HGS, I'll trust you on this one (I blame myself for lack of CI here),
- not checking if it's actually performing algorithm details,
- no CI :(
Overall it looks pretty ok, but:
- the licensing thing, as I believe your intent was bit different. If you insist and convince me&other (3) contributors that MIT > GPL for this repo, we could do relicensing,
- plotting in
algorithms/FVMOEA/indicators.py— this looks like a script, not a proper part ofalgorithmsmodule tree, - mostly minor stuff here & there,
- I'm really not sure if ABC should be used. Made notes whenever they occur.
I would like you to answer to my comments and/or apply suggested fixes.
| @@ -0,0 +1,21 @@ | |||
| MIT License | |||
There was a problem hiding this comment.
As far as I'm aware, this is moot. Having in mind that MIT is compatible with GPL-3 [link]:
- that means that you can use them both (there's no contradiction),
- that also means that result is licensed under GPL-3.
We originally had this repo under BSD-3 (IIRC), but transitioned to GPL (see 86e9965). I think it's still an open debate if we could/should migrate to less requiring licenses. Awaiting your comment here.
| Based on Jiang, Siwei, et al. "A simple and fast hypervolume indicator-based multiobjective | ||
| evolutionary algorithm." IEEE Transactions on Cybernetics 45.10 (2015): 2202-2213. | ||
| http://www3.ntu.edu.sg/home/ZhangJ/paper/fvmoea.pdf | ||
| """ |
There was a problem hiding this comment.
(minor) I suppose that you intended this to be a docstring for whole class and not __init__?
|
|
||
| @abstractmethod | ||
| def run(self): | ||
| pass |
There was a problem hiding this comment.
(minor/preference/suggestion) I usually prefer to give absurd code absurd values — like raise RuntimeError("Absurd: this should never happen") or something.
|
|
||
| def print_indicator_values(self, population: Population) -> None: | ||
| print(f'Function evaluations: {self.function_evaluations}') | ||
| population_objectives = list(map(attrgetter('objectives'), population)) |
There was a problem hiding this comment.
(uniformity) I would suggest the following to keep new code more uniform with existing codebase:
population_objectives = [
self.objectives(individual)
for individual in population
]I believe that this may be bit more Pythonic [citation needed].
| from metrics.metrics_utils import filter_not_dominated | ||
|
|
||
|
|
||
| class Algorithm(ABC): |
There was a problem hiding this comment.
I'm not sure what's the main use for ABC/abstractmethod here. Name of class suggests broad use, but I see the only merit of this: doing workarounds for Pythonic lack of enforcing abstract methods.
If that is the case, then I would further argue that this code is trivial / short, and only ever used in FVMOEA. Because of this I see zero value in splitting __init__ code across two classes or separating print_indicator_values into base class. The current codebase's approach is to use duck typing instead: if has run(self) then it'll work; no static checks.
NB. The discussion would be completely different in a language that promotes static checking.
| ] | ||
|
|
||
|
|
||
| class UF(Problem, metaclass=ABCMeta): |
| from algorithms.FVMOEA.population import Individual | ||
|
|
||
|
|
||
| class Problem(ABC): |
|
|
||
| class UF1(UF): | ||
| def import_problem(self): | ||
| from problems.UF1.problem import fitnesses, dims, pareto_front |
There was a problem hiding this comment.
The name of this method suggests some… problems. Care to elaborate on the issue & on the workaround?
There was a problem hiding this comment.
(arguable) A guess: if you had a name conflict & nice reexports, then maybe the following would be more clear? It will do :
import typing
import problems.UF1.problem as UF1p
import problems.UF2.problem as UF2p
# …snip…
class ProblemDescription(typing.NamedTuple):
dims: List[Tuple[int]]
fitnesses: List[Callable[[List[float]], float]] # please check this
pareto_front: List[List[float]]
# note that those will be re-exported
uf1 = ProblemDescription(UF1p.dims, UF1p.fitnesses, UF1p.pareto_front)
uf2 = ProblemDescription(UF2p.dims, UF2p.fitnesses, UF2p.pareto_front)
# …snip…
class UF(Problem):
def __init__(self, variables_count: int, problem_description):
self.dims = problem_description.dims
self.fitnesses = problem_description.fitnesses
self.pareto_front = problem_description.pareto_front
# rest of the __init__…Same would be possible for ZDT class.
| return dims, fitnesses, pareto_front | ||
|
|
||
|
|
||
| class ZDT(Problem, metaclass=ABCMeta): |
There was a problem hiding this comment.
Old-way to ABC. Also: ABC/abstractmethod argument here.
| from algorithms.FVMOEA.population import Individual, Population | ||
|
|
||
|
|
||
| class Selection(ABC): |
There was a problem hiding this comment.
ABC/abstractmethod argument here.
Implementation of FV-MOEA based on Jiang, Siwei, et al. "A simple and fast hypervolume indicator-based multiobjective evolutionary algorithm." IEEE Transactions on Cybernetics 45.10 (2015): 2202-2213. http://www3.ntu.edu.sg/home/ZhangJ/paper/fvmoea.pdf