Refactoring of DoEStrategy data model class
#448
Conversation
|
@jduerholt @Osburg i made some changes. We still want to support the old interface with |
|
@jduerholt @Osburg This PR is now ready for merging. Do you approve? |
|
We first review :-) Thank you very much. I try to have a look at it on the weekend. @Osburg would be good if you also have a look. |
|
There are failing tests due to a new sklearn version, I already went into the rabbit hole. Will post an issue later. |
There was a problem hiding this comment.
@dlinzner-bcs Thanks!!! :)) <3 looks good to me, left some minor comments. Feel free to ignore them if they are not helpful.
Cheers, Aaron :)
| @@ -83,6 +74,71 @@ def evaluate_jacobian(self, x: np.ndarray) -> np.ndarray: | |||
| def _evaluate_jacobian(self, x: np.ndarray) -> np.ndarray: | |||
| pass | |||
|
|
|||
| @abstractmethod | |||
There was a problem hiding this comment.
Are these functions needed in the Objective class? Since the SpaceFilling objective actually does not have to implement any of these functions, right? (The only function that is used for any objective is get_model_matrix() in the exit message find_local_max_ipopt() i think, but imo it is not necessary there as well...). What is your opinion?
bofire/strategies/doe/objective.py
Outdated
| @@ -493,19 +552,66 @@ def _convert_input_to_tensor( | |||
| ) | |||
| return torch.tensor(X.values, requires_grad=requires_grad, **tkwargs) | |||
|
|
|||
| def get_model_matrix(self, design: pd.DataFrame) -> pd.DataFrame: | |||
There was a problem hiding this comment.
This could then also be removed
bofire/strategies/doe/design.py
Outdated
| @@ -572,7 +199,7 @@ def find_local_max_ipopt( | |||
| if _ipopt_options[b"print_level"] > 12: # type: ignore | |||
There was a problem hiding this comment.
In my opinion this could be removed. The objective criterion can be viewed via the IPOPT output and any other criterion can also be calculated with little effort by creating another Objective. What do you think? :) --> then there is also no need anymore to implement get_model_matrix() for SpaceFilling.
Also the metrics(), g_optimality(), a_optimality(), d_optimality() functions in doe utils could be deleted then since they are not used anywhere else.
There was a problem hiding this comment.
Many thanks Aaron for the review :)
|
Thanks @Osburg, I will finally have look tmr. Sorry @dlinzner-bcs ! |
|
why is the nbstripout precommit hook not being executed properly? The notebook output should not be there. Right now there are some images and outputs referencing local paths on someone's machine edit: ah, never mind... we use nbstripout with the argument to keep outputs. But the warnings should still be removed I think |
There was a problem hiding this comment.
Why are you adding this? We removed it during the hackathon in favor of having everything in the pyproject.toml.
|
|
||
| objective: OptimalityCriterionEnum = OptimalityCriterionEnum.D_OPTIMALITY | ||
|
|
||
| transform_range: Optional[Bounds] = None |
There was a problem hiding this comment.
Where is the transform_range gone?
| n_experiments: Optional[int] = None, | ||
| delta: float = 1e-7, | ||
| n_experiments: int, | ||
| criterion: Optional[AnyOptimalityCriterion] = None, |
There was a problem hiding this comment.
Why is the criterion here optional? We always need one, or?
There was a problem hiding this comment.
Can we get rid of this functional strategy here, and just map the data model of the SpaceFillingStrategy to the DoeStrategy in the mapper?
There was a problem hiding this comment.
I would have liked to use the SpaceFillingStrategy as it is to generate the design space grid for the I-optimality criterion. To avoid circular imports it would be nice if the space filling strategy would be available indepent of the DoeStrategy (@dlinzner-bcs is it okay if I add a commit for the I-optimality - now compatible with your changes - criterion to this branch?).
Only if there are no good reasons against keeping it this way ofc @jduerholt .
edit: nvm
There was a problem hiding this comment.
We should also test the optimality criteria data models for serialization and desirilization, should I implement this into this PR? One has to register a few new fixures for this.
This PR provides a draft for the restructured
DoEStrategy.The following things changed:
objectiveis nowcriterionto prevent confusion with the objectives of the output features.DOptimalityare not anymore enums, but classes, for exampleDOptimalityCriterion. This allows for the clean implementation of criterion specific attributes.DoEStrategybut instead an attribute of thecriterion.When the functional part of the DoE module is refactored, we should have a mapper which takes the criterion and domain and returns a functional criterion class which provides then the actual objective funtion for the actual ipopt optimizer.
On the long run, we can then also provide a common base class on the data model level for both the
DoEStrategyand theSpaceFillingStrategy. In case of the latter one the only allowed criterion will then be theSpaceFillingCriterionwhich has noformulaattribute.@Osburg: What do you think?