Closed Saethox closed 1 year ago
While this sounds really good in general, would it be possible to split this PR in smaller ones, each focussed on a specific aspect? Reviewing everything as a whole is a bit overwhelming and particularly prone to errors. Generally, we need a better PR workflow.
If its not (easily) possible, we can leave the PR as is, but next time, smaller ones would be better.
@Saethox how much work would it be to split this up? Or at least move everything, where it is convenient, into separate PRs. For example, removing the old problem modules could easily be separated, right?
While this sounds really good in general, would it be possible to split this PR in smaller ones, each focussed on a specific aspect? Reviewing everything as a whole is a bit overwhelming and particularly prone to errors. Generally, we need a better PR workflow.
I know, right 🙈. I was a little much too focused on getting it all to work together.
how much work would it be to split this up? Or at least move everything, where it is convenient, into separate PRs. For example, removing the old problem modules could easily be separated, right?
Splitting the problems is easily done, the other changes are more cumbersome. I'm in no real hurry to merge this (maybe before GECCO), so I could probably split this PR into three/four incremental PRs:
par_experiment
)Depending on how we want to handle the problem dependencies (mahf-tsplib
-> mahf
, mahf
-> mahf-tsplib
, mahf-problems
-> mahf-tsplib
-> mahf
), the problem changes could be moved into the mahf-problems
repo at this point.
We can then just use this draft as a proof that all of the concepts do work together, so I guess that's nice to know 🤷♂️.
I think 3 or 4 PRs would be okay.
I'm in no real hurry to merge this (maybe before GECCO)
Well, I would prefer as soon as possible, but before GECCO would be okay. I have to start implementing #75 and I also need to play around with the logging a bit to get data for the evaluation (so I can finally implement it completely) and I fear if I start before this PR is done, I will cause additional problems.
I have to start implementing https://github.com/mahf-opt/mahf/issues/75
That's not really a problem, porting components to the new state management and returning a result instead of panicking is done in a few minutes, It's not like the functionality of the components gets any different through this, especially for specialized operators.
I also need to play around with the logging a bit to get data for the evaluation
Again, the general concept of logging is not that different, it's just more flexible than before. The log format is also exactly the same.
Obsolete with #176 and #178
I'm making this a draft for now, so we can discuss the design decisions before anything is merged. This is a PR that emerged from some experimental changes I wanted to try out and have integrated before we publish on crates.io, and it subsequently got somewhat out of control (the line diff scares me).
In general, I'm pretty happy with how it turned out, but there is still much work to do apart from all of this.
Documentation
This PR should include
lib
,components
,conditions
,state
,logging
,heuristics
, again with examplesI just didn't want to spend time documenting stuff we might change, so I put it off until now. It should definitely be part of this PR though.
Rustfmt
I added a
rustfmt.toml
with the options mentioned in #164, and adapted theci.yml
to use nighlyrustfmt
, because these options are nightly.clippy
,check
, andtest
are still performed onstable
just like before. I just can't go back to the old imports after I have seen the beauty of what nightlyrustfmt
is capable of.Changes
Problems
Evaluator
component (nowPopulationEvaluator
) was changed from accessing theEvaluatorInstance
in theState
to having aT: Evaluator
type parameterEvaluator
s in the state at the same time, and decouplesEvaluator
andProblem
somewhatProblem::default_evaluator
was removed in favor of aTryDefault
bound onEvaluator
ObjectiveFunction
trait is a simpler version ofEvaluator
for problems where the objective function can be easily specified, andSequential<P>
andParallel<P>
offer a default implementation of a sequential and parallelEvaluator
, respectively, whenP
implementsObjectiveFunction
LimitedVectorProblem
depends onVectorProblem
, etc.) and a separate TSP trait to access distance informationState
MutState
is no more:State
now allows for proper (mutable) borrowing of custom state at the same time (makingRef
andRefMut
part of the public API is kind of annoying, but it's still so much better than before IMO)HashMap::entry
State<P>
is now only a thin wrapper around theStateRegistry
, to allow accessing custom state without having to addP
as a type parameter everywhere where it's really not necessary (especially relevant for the newExtract
mechanism)Extract
,ExtractRef
, andExtractRefMut
now make it possible to access custom state without caring what the exact wrapper type is&self
) for several reasons: dealing with boxed extractors was a major hassle, they are meant to only map values, not really modify them, and adding them through type parameters allows a pretty readable syntax, imoIdFn<T>
makes it possible to retrieve any custom stateValueOf<T>
makes it possible to retrieve theDeref::Target
of a wrapper type, which is really handyLessThan<ValueOf<Iterations>>::new(10)
instead of implementing separateFixedIterations
,FixedEvaluations
, etc. conditionsLinear<I, O>
component taking an input value from the extractorI
and mapping it linearly onto the custom state extracted byO
State
now definestry_
methods which return aResult<_, StateError>
Components and Conditions
initialize
was renamed toinit
require
phase betweeninit
andexecute
, where only requirements can be definedswarm
andgenerative
modules are now meant to hold PSO and ACO-like components, respectively, if we add moreComponent
andCondition
methods now return aResult
eyre
was used as error library because it has better backtrace information thananyhow
, and a custom error type would be much more hasslefrom_params()
method, which returns the unboxed component/conditionUniformMutation
andNormalMutation
store theirMutationRate
in the state, but if they use the same type they overwrite each otherMutationRate
generic over the type of the component that stores it -->MutationRate<UniformMutation>
Identifier
type -->UniformMutation<I: Identifier = Global>
Identifier
isGlobal
, but can be replaced withA
,B
, etc., or own types<>
if the default identifier is used, but it's not really a problem (should be documented though because it took me a while to figure it out)termination
was removed, there are now only conditionsLessThan
,EveryN
, andChangeOf
conditions are now very flexible in combination with extractorsMutator
,Initializer
, etc. were replaced with functions doing the same thinginit
orrequire
was very awkward for components only implementing the simpler trait, and would require adding these methods to each simpler traitSelector(ComponentImplementingSelection)
, which made an implementation detail part of the serializationSelector
were not easily retraceable to the component that was wrappedComponent
, which is a little bit more code, but has none of these flawsinit
orrequire
, we could add a derive macro that accepts the translator function and simply calls it in theexecute
implementationLogging
tracking
was renamed intologging
Trigger
was removed in favor of just usingCondition
sCondition
s have aSerialize
bound, but having a separateTrigger
either leads to much code duplication or very weird conversion codeTrigger
, it might as well be reused as aCondition
, so it can be one in the first placeExtractor
(nowEntryExtractor
) trait is now built on top of theExtract
API, which again is very comfortable because everything can be reusedLogSet
is nowLogConfig
, because this makes more sense IMOOther
par_experiment
helper was added for dealing with experiments, this could (and probably should) be extended into a whole experiment framework, maybe some sort ofExperiment
builder with a bunch of options