mahf-opt / mahf

A framework for modular construction and evaluation of metaheuristics.
GNU General Public License v3.0
10 stars 0 forks source link

Integrate most of the changes from PR #173 #178

Closed Saethox closed 1 year ago

Saethox commented 1 year ago

It's probably best to check out the branch locally and review this inside the IDE or run cargo doc, as this PR includes extensive documentation (yet there is still enough missing), which is not very nice to review on GitHub.

I decided to add the logging refactor here already because entangling this is not really worth it, as I would either merge incorrect documentation on other items or spend time rewriting documentation which is then immediately updated.

Changes

Closes #161 Closes #160 Closes #157 Closes #84

New concepts

Identifiers

Identifiers are zero-sized marker types that allow storing the same type T multiple times inside State by adding a I: Identifier type parameter together with a PhantomId to the type, which is handy in a bunch of places.

Lenses

Documentation

Problems

State

Components and Conditions

Logging

luleyleo commented 1 year ago

I think I'll find time for it this weekend :)

Saethox commented 1 year ago

The documentation is missing for many parts, some modules don't have any.

Writing (good) documentation, unfortunately, takes so much time that I wanted to open this PR even if some stuff is missing documentation. I planned to do another PR focusing on documentation, but I could also make that part of this PR if you think that's better.

I'm not convinced that lenses are a good idea.

I agree that they add complexity, but they are just so elegant. They allow massive code reuse and integrate really well with logging. Why would I want a new component every time I want some parameter to decrease with iterations if I can just specify what parameter should decrease? But they certainly should be really well documented, which they are currently not really.

While I do think lenses are powerful and useful, I don't think they are easy to understand. Especially, people without a mathematical background seem to struggle with it.

One advantage of developing a "scientific" library is that we can assume some mathematical background of people who might want to use MAHF, so the benefit really outweighs the additional complexity (at least in my opinion), in this case. And not that we have to copy what others do, but e.g. CIlib also extensively relies on functional programming concepts like lenses.

HeleNoir commented 1 year ago

The documentation is missing for many parts, some modules don't have any.

Writing (good) documentation, unfortunately, takes so much time that I wanted to open this PR even if some stuff is missing documentation. I planned to do another PR focusing on documentation, but I could also make that part of this PR if you think that's better.

I wouldn't mind if the documentation is completed in another PR. However, (thinking about presenting MAHF at GECCO) the parts that are most difficult to understand should have documentation.

I'm not convinced that lenses are a good idea.

I agree that they add complexity, but they are just so elegant. They allow massive code reuse and integrate really well with logging. Why would I want a new component every time I want some parameter to decrease with iterations if I can just specify what parameter should decrease? But they certainly should be really well documented, which they are currently not really.

While I do think lenses are powerful and useful, I don't think they are easy to understand. Especially, people without a mathematical background seem to struggle with it.

One advantage of developing a "scientific" library is that we can assume some mathematical background of people who might want to use MAHF, so the benefit really outweighs the additional complexity (at least in my opinion), in this case. And not that we have to copy what others do, but e.g. CIlib also extensively relies on functional programming concepts like lenses.

I'm quite torn on the topic of lenses. I can see that they are complex to really understand, but my feeling was that even without proper understanding, you can still implement and use them (maybe I will try later and see what happens). I just don't know if that's what we want. However, I also think that we can expect some background knowledge of possible users. So most important would definitely be a proper documentation, preferably in this PR.

And maybe a stupid idea: can we offer both options, i.e. lenses and components, without breaking the general logging implementation (or without having to do too much adaptation of the code)? Then people could choose depending on their experience when they want to write own extensions and we can use lenses.

Saethox commented 1 year ago

I'm quite torn on the topic of lenses. I can see that they are complex to really understand, but my feeling was that even without proper understanding, you can still implement and use them (maybe I will try later and see what happens). I just don't know if that's what we want. However, I also think that we can expect some background knowledge of possible users. So most important would definitely be a proper documentation, preferably in this PR.

I think we can also hide most of the complexity of lenses by just adding methods for instantiating components with the lenses they are most commonly used with. For example, I added LessThanN::iterations and LessThanN::evaluations methods, which hide that internally just a ValueOf lens is created with the corresponding custom state.

And maybe a stupid idea: can we offer both options, i.e. lenses and components, without breaking the general logging implementation (or without having to do too much adaptation of the code)? Then people could choose depending on their experience when they want to write own extensions and we can use lenses.

Lenses in MAHF are not fully features lenses, but really basically what Extractor was in the old logging with a different name (my first implementation was actually named Extract when I didn't realize yet I'd just reimplemented a kind of lens) and a little bit more generic. You get the State as an argument, extract whatever you want from it and return it, that's basically it. And you can use it both in logging and components, in contrast to Extractor.

luleyleo commented 1 year ago

If we assume competent users, then lenses should be nice. Having simple helper functions such as LessThanN::iterations makes it much nicer to write / read too.