Conversation
4fee41e to
c052743
Compare
4669026 to
b7e5bec
Compare
1664e90 to
5a43378
Compare
5a43378 to
a9d8084
Compare
AnastasiiaKabeshova
left a comment
There was a problem hiding this comment.
Great code organization improvement :)
Just one question for myself: Why was Optuna replaced? (Performance, dependency reduction, or other reasons?)
pybandits/utils.py
Outdated
| return result.x | ||
| else: | ||
| logger.warning(f"Optimization failed: {result.message}") | ||
| return None |
There was a problem hiding this comment.
When optimization fails, maximize_by_quantity returns None
Did you check that all callers handle None properly?
Whether a more specific exception would be better?
There was a problem hiding this comment.
Good point, I added an appropriate exception handling.
| QuantitativeProbabilityWeight = Tuple[Tuple[Tuple[Float01, ...], ProbabilityWeight], ...] | ||
| QuantitativeMOProbability = Tuple[Tuple[Tuple[Float01, ...], List[Probability]], ...] | ||
| QuantitativeMOProbabilityWeight = Tuple[Tuple[Tuple[Float01, ...], List[ProbabilityWeight]], ...] | ||
| QuantitativeProbability = Callable[[np.ndarray], Probability] |
There was a problem hiding this comment.
The name of the type is misleading...
maybe it should be: QuantitativeProbabilityFunction?
There was a problem hiding this comment.
@ronshiff1 Maybe ContinuousProbability? I just didn't want it to be too long.
| @@ -68,6 +78,12 @@ class QuantitativeModel(BaseModelSO, ABC): | |||
| def sample_proba(self, **kwargs) -> List[QuantitativeProbability]: | |||
There was a problem hiding this comment.
Please explain why list. For Multi objective?
There was a problem hiding this comment.
If I understand correctly - it's for different segments?
I think that it is very specific to the zooming algorithm, and it should be function which receives an input, and outputs one function, and not a list
the zooming model should create this function and handle the segmentation internally (I also commented on the first sample_proba implementation)
There was a problem hiding this comment.
The list is not because of zooming. It returns a list whose length is as the number of samples.
There was a problem hiding this comment.
I fixed the docstring to make it explicit.
a9d8084 to
7f6a131
Compare
| for segment in segment_probabilities.keys(): | ||
| if quantity in segment: | ||
| segment_probas_for_segment = segment_probabilities[segment] | ||
| return segment_probas_for_segment[sample_idx][output_index] # Probability or weight |
There was a problem hiding this comment.
Why not directly return segment_probas_for_segment[sample_idx] which is tuple?
(so def create_probability_or_weight_function(
sample_idx: NonNegativeInt
) -> Tuple[QuantitativeProbability, QuantitativeWeight]:
There was a problem hiding this comment.
Then in the cmab class you would need to separate them to list of probabilities and list of weights.
So I preferred to do it in advance.
| return annotation | ||
|
|
||
| @classmethod | ||
| def _normalize_field(cls, v: Any, field_name: str) -> Any: |
There was a problem hiding this comment.
I use it in validators of "pre" behavior to get the default value of an ungiven attribute
| rewards: Union[List[BinaryReward], List[List[BinaryReward]]] | ||
| The reward for each sample. | ||
| """ | ||
| super()._quantitative_update(quantities, rewards) |
There was a problem hiding this comment.
this:
quantities: Optional[List[Union[float, List[float], None]]],
rewards: Union[List[BinaryReward], List[List[BinaryReward]]],
doesn't conform with the ZoomingModel _quantitative_update:
def _quantitative_update(self, quantities: List[Union[float, np.ndarray]], rewards: List[BinaryReward], **kwargs):
There was a problem hiding this comment.
Wow! Nice catch. Fixed to: Optional[List[Union[float, List[float], None]]]
### Changes: * Moved `_normalize_field` method to `PyBanditsBaseModel` for default value normalization of optional fields. * Unified quantitative actions interface to use callables for general form, rather than floats for zooming. * Improved tests for action selection and quantitative actions, ensuring proper handling of constraints and expected results.
7f6a131 to
2c00401
Compare
| self, | ||
| p: ActionRewardLikelihood, | ||
| actions: Optional[Dict[ActionId, BaseModel]] = None, | ||
| ) -> ActionId: |
There was a problem hiding this comment.
say, shouldnt it be
-> Union[ActionId, QuantitativeActionId]?
| "\n", | ||
| " # Observe reward\n", | ||
| " rewards = [\n", | ||
| " reward_function(chosen_action, chosen_quantity, current_context[0])\n", |
There was a problem hiding this comment.
isn't current_context[0] a bug?
it uses the first context for all samples
Changes:
_normalize_fieldmethod toPyBanditsBaseModelfor default value normalization of optional fields.