Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Budget allocation and optimal point estimations #329
Budget allocation and optimal point estimations #329
Changes from 4 commits
b8555cd
8cd82ce
2bbd10f
f449f6d
01c1abc
17d9da6
f0ad14a
56b9a37
7a595e8
a2f31d4
2e008ea
265ab69
abcde2d
d9ca6f5
036bc1a
c492005
98d0418
7d66ac9
2076ff3
03fa95f
3ae0a67
427dd19
281c379
b9f6fc9
6f8294c
7a5137d
a5ab632
475504b
5cab673
9f99c46
e85323c
e532ac9
d3fce02
e240817
bf0c73d
a019e37
af344eb
e23ebd7
dba0f24
cd9a40c
b3fa3b3
c48691e
a37cbde
f5fa1eb
05d6bb5
7305590
84e9f73
9e34c31
7da6986
db5bfcd
acb343e
06adcdd
df5d883
1ebeddb
a572a3a
5d2832f
38aae8e
bd4cb15
c679b88
331971a
2af4f0a
51afe7c
975a89d
cef8c86
1c4f093
7562714
aeb7faf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why do you use these 0.99 and 0.01 ?
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.
It's a practical solution because the Michaelis-Menten equation is given by:
y = (k + x) / (L * x)
wherek
represents the substrate concentration at which they
is half ofL
. In other words, whenx = k
andy = L/2
. Asx
increases the equation approaches:y≈L
.As obvious because
y
becomes saturated; adding morex
doesn't significantly increasey
. The valueL
is an asymptote for the function, meaning the curve approachesL
but never quite reaches it.Using 0.99 and 0.01 calculates the
x
point wheny
is 99% ofL
. Simplified, it results in99k
.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.
Thanks! Do you want to add a shorter summary to the doc strings? :)
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.
If we have
Optional
type it mean that they can beNone
? Otherwise is not anOptional
type. See https://mypy.readthedocs.io/en/stable/kinds_of_types.htmlThere 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.
Totally correct, in this case, parameters should not be optional, only budget bounds. Change budget bounds to be optional and modify parameters based on this.
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.
returns dict or data frame?
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.
Corrected.
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 rename this method as
compute_channel_plateat_points_original_scale
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 switch to
compute_channel_curve_parameters_original_scale
given the fact we can use a differents curve fit now. What do you think?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.
Maybe we can use a simple dict comprehension
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.
Good catch!
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.
add return type hint and arguments type hint
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.
Sure!
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.
add type hints
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.
From the example notebook I get
Should we add the expected total value instead of
NaN
?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.
Correct, already working as expected.
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.
This is also useful as a dedicated saturation function in mmm.transformers
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 like this idea, checking Google MMM lightweight they had included several model configurations (Hill, Carryover, Adstock) which you could choose to define Saturation and Lagging.
Example:
They refer to the following wiki where the saturation mentioned it is very similar to the
Michaelis Menten
. I could imagine we can implement something similar here, in order to be more extensive to how different existing data sets can fit better to different saturation and lagging functions depending on their own conditions.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.
type hints