-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Permit BudgetOptimizer.allocate_budget()
to take x0
as an argument
#1565
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1565 +/- ##
=======================================
Coverage 93.51% 93.51%
=======================================
Files 55 55
Lines 6341 6342 +1
=======================================
+ Hits 5930 5931 +1
Misses 411 411 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# Merge with defaults (preferring user-supplied keys) | ||
minimize_kwargs = {**self.DEFAULT_MINIMIZE_KWARGS, **minimize_kwargs} | ||
# 5. Construct the initial guess (x0) if not provided | ||
if "x0" not in minimize_kwargs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think x0 should be hidden in minimize kwargs, I would make it an explicit argument of allocate_budget
minimize_kwargs["x0"] = np.ones(budgets_size) * ( | ||
total_budget / budgets_size | ||
) | ||
minimize_kwargs["x0"] = minimize_kwargs["x0"].astype( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would check if it has the expected shape (budgets_size,)
, as the compiled functions have trust_input=True
and won't check if x0 has the promised shape
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good point here!
Just a hardcoded check based on the data, we could change this to be more dynamic and check if values are equal based on the data, not hardcoded 50/50 🙃 Does that makes it clear? |
self._total_budget.set_value(np.asarray(total_budget, dtype="float64")) | ||
|
||
# coordinate user-provided and default minimize_kwargs | ||
if minimize_kwargs is None: | ||
minimize_kwargs = self.DEFAULT_MINIMIZE_KWARGS.copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: no real need to copy
minimize_kwargs = self.DEFAULT_MINIMIZE_KWARGS.copy() | |
minimize_kwargs = self.DEFAULT_MINIMIZE_KWARGS |
@@ -391,8 +392,11 @@ def allocate_budget( | |||
- If None, default bounds of [0, total_budget] per channel are assumed. | |||
- If a dict, must map each channel to (low, high) budget pairs (only valid if there's one dimension). | |||
- If an xarray.DataArray, must have dims (*budget_dims, "bound"), specifying [low, high] per channel cell. | |||
x0 : DataArray, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be a np.ndarray not a DataArray
self._budgets_flat.type.dtype | ||
) | ||
# if x0 arg is provided, validate shape | ||
elif x0.shape != (budgets_size,): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also validate dtype
Just need to write some tests. |
f"""The shape of 'x0' {x0.shape} does not match the expected shape {(budgets_size,)}.""" | ||
) | ||
# if x0 arg is provided, validate dtype | ||
elif np.issubdtype(x0.dtype, np.number): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically the x0.dtype
must match self._budgets_flat.type.dtype
. I think there's a shortcut you can use for the shape and dtype validation: x0 = self._budgets_flat.type.filter(x0)
, which will do any allowed casting or raise if things cannot be safely cast (and also checks shape)
Description
BudgetOptimizer.allocate_budget()
now acceptsx0
as an argument.Related Issue
BudgetOptimizer.allocate_budget()
doc string doesn't specify default value of x0 #1564Checklist
pre-commit.ci autofix
to auto-fix.x0
is now accepted via theminimize_kwargs
(as the docstring implies is possible). I looked into adding a corresponding test, but these lines intests/mmm/test_budget_optimizer::test_allocate_budget_custom_minimize_args()
confuse me. Any guidance?📚 Documentation preview 📚: https://pymc-marketing--1565.org.readthedocs.build/en/1565/