Opened 8 years ago

Closed 8 years ago

#1430 closed defect (done)

BestKnownQuality should be an optional value parameter in SingleObjectiveProblem<T, U>

Reported by: abeham Owned by: abeham
Priority: high Milestone: HeuristicLab 3.3.4
Component: Optimization Version: 3.3.4
Keywords: Cc: mkommend

Description (last modified by abeham)

Currently it's a ValueParameter that cannot be set to null if there is no best known quality yet.

Change History (13)

comment:1 Changed 8 years ago by abeham

  • Cc mkommend added
  • Description modified (diff)
  • Summary changed from BestKnownQuality should be an optional value parameter in SingleObjectiveProblem<U, V> to BestKnownQuality should be an optional value parameter in SingleObjectiveProblem<T, U>

I tried to implement this quickly, by swapping the ValueParameter<DoubleValue> with an OptionalValueParameter<DoubleValue>, but noticed that there's an ugly side effect in making this change backwards compatible:

When loading the problem, the parameter needs to be removed and readded in AfterDeserialization in case it is of type ValueParameter<T, U>. This however requires that SingleObjectiveProblem.AfterDeserialization needs to run before any other AfterDeserialization hook in the derived classes are executed. If it runs afterwards, there might already be an event handler registered on the parameter which to my knowledge cannot be re-registered and the derived class never gets the events. Unfortunately, as far as I know there is no guarantee in the order of execution of the hooks.

I can think of three possible solutions:

  1. Make AfterDeserialization a protected virtual method and instead of each subclass having its own private method make an override and do the base call.
  2. Implement a guarantee that the hooks defined in the base type are executed before the hooks in the derived type at the cost of some performance in the persistence.
  3. Just remove and readd the parameter and not care about any registered event handlers since there's hardly a problem that has or needs to register an event handler on that specific parameter. Of course here we only guarantee backwards compatibility for problems in the trunk.

Sidenote: The parameter properties (in general) should return IValueParameter<T> instead of the specific type.

comment:2 follow-up: Changed 8 years ago by swagner

As far as I know, only SingleObjectiveClassificationProblem and SymbolicClassificationProblem derive from SingleObjectiveProblem at the moment. Furthermore, I do not think that we really run into a backwards compatibility problem here. If we simple change the type, it will not be possible to set the best known quality back to null in previously persisted and restored classification problems. In my opinion this would not be a severe issue. Or do I miss something?

Anyhow, I think we should implement the second suggested solution as after deserialization hooks should behave the same way as constructors do (i.e., they should be called in base classes first).

comment:3 Changed 8 years ago by mkommend

Do we really want to reset the best found quality to null. Would it not be sufficient to remove the default value in the ctor of the value parameter?

comment:4 Changed 8 years ago by abeham

When I'm loading another problem instance which doesn't have a best known solution/quality, I'd like to set the value of this parameter to null. The other option would be to set it to double.MaxValue (for a minimization problem), but I don't really like that.

If you look at the TSP the BestKnownQuality is also an OptionalValueParameter there.

comment:5 in reply to: ↑ 2 Changed 8 years ago by swagner

Replying to swagner:

Anyhow, I think we should implement the second suggested solution as after deserialization hooks should behave the same way as constructors do (i.e., they should be called in base classes first).

According to epitzer calling deserialization hooks is already implemented in that way. Before and after deserialization hooks are called on base classes first.

Last edited 8 years ago by swagner (previous) (diff)

comment:6 Changed 8 years ago by swagner

  • Owner changed from swagner to abeham
  • Status changed from new to assigned

abeham, can you please change the BestKnownQualityParameter in SingleObjectiveProblem into an OptionalValueParameter and implement the required after deserialization code as suggested.

comment:7 Changed 8 years ago by abeham

  • Status changed from assigned to accepted

comment:8 Changed 8 years ago by abeham

  • Owner changed from abeham to mkommend
  • Status changed from accepted to reviewing

r5872

  • Changed BestKnownQuality parameter to an OptionalValueParameter

comment:9 Changed 8 years ago by mkommend

  • Owner changed from mkommend to abeham
  • Status changed from reviewing to readytorelease

Thanks for implementing this.

comment:10 Changed 8 years ago by mkommend

  • Owner changed from abeham to mkommend
  • Status changed from readytorelease to assigned

comment:11 Changed 8 years ago by mkommend

  • Owner changed from mkommend to abeham
  • Status changed from assigned to reviewing

r5899: Changed access modifier of AfterDeserializationHook to private.

comment:12 Changed 8 years ago by abeham

  • Status changed from reviewing to readytorelease

ok, thx

comment:13 Changed 8 years ago by swagner

  • Resolution set to done
  • Status changed from readytorelease to closed
  • Version changed from 3.3.3 to 3.3.4
Note: See TracTickets for help on using tickets.