-
Notifications
You must be signed in to change notification settings - Fork 3
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
Define computation in a function in examples #17
Conversation
61b845f
to
80b3231
Compare
69540e9
to
cb0f384
Compare
c9a2473
to
f098f09
Compare
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.
@JernKunpittaya I've replaced verifier models with computation functions in all examples. Just as the comments above (and places marked with "FIXME") mentioned: results from where+xxx.ipynb
are wrong since we need to add State.where
and modify the existing statistics operations to make the result correct
" # FIXME: should be replaced by `s.where` when it's available. Now the result may be incorrect\n", | ||
" filter = (y < 20)\n", | ||
" # FIXME: not sure how to do filtering correctly here\n", | ||
" filtered_x = torch.where(filter, x, 0.0)\n", |
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 should be wrong for calculating filtered_x
but I'm not sure how to do it since in the example torch.where
is done after torch.cat
. Do you have any thoughts on this, or I'm thinking we can just leave it for now and change it after State.where
is supported?
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.
Thx Kevin! Nw, will open a new PR that specifically address where op!
" x = data[0]\n", | ||
" # FIXME: should be replaced by `s.where` when it's available. Now the result may be incorrect\n", | ||
" filter = (x > 5)\n", | ||
" filtered_x = torch.where(filter, x, 0.0)\n", |
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.
We need other filters like fil_mean_X
and fil_std_X
to make it run correctly. Or probably we can fix it later when State.where
is supported?
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.
Yes, or maybe we can encapsulate the logic in s.mean to detect if it is addressing the filter data or not. Will open a new PR for this
…on case, mod supported in median
Tmp/fix/use computation in examples
What is the PR doing?
In #5 , we allow users to define computation in a function, but we haven't changed the examples.
How to fix it?
Define computation in functions instead of model classes in the examples. However, this PR is not ready yet since our current statistics functions don't work correctly with filtered tensors. For example,
if
X
has 300 elements, elements filtered out might be 200, butfil_X
will still be 300 elements ands.mean
result will be incorrect since the number of elements should be 200 instead of 300.To-Dos
So the following examples stay unchanged and should be changed:
examples/where+geomean
examples/where+mean
examples/where+median
examples/where+mode
examples/where+regression
examples/where+stdev