-
Notifications
You must be signed in to change notification settings - Fork 42
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
use tf.debugging for search space assertions #299
Conversation
|
||
return space | ||
tf.debugging.assert_positive(other, message="Exponent must be strictly positive") | ||
return reduce(operator.mul, [self] * other) |
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.
felt like doing this, can undo if you'd prefer
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.
definitely more elegant!
am I missing something or assert_positive
means >0 while here we want >=1?
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.
you're right. maybe i though positive meant strictly positive
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.
scrap that ... > 0 is the same as >=1 for integers, and that's also what assert_positive
does
trieste/space.py
Outdated
tf.debugging.assert_equal( | ||
shapes_equal(value, self._lower), | ||
True, | ||
message="value must have same dimensionality as search space" | ||
) |
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 a bit of a hack, to get around the fact that tf.debugging.assert_shapes
considers 0 and [0] to have the same shape
810eb4f
to
0a5cf89
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.
so we should use tf.debugging asserts throughout the code, rather than plain asserts (and ifelse statements with raising errors/warnings/etc)?
|
bd6f806
to
fe76cf2
Compare
I see, I didn't know about the difference with respect to compilation. Regarding tf.function, at the moment it is used only in model optimization, is that correct? |
yes
I don't think
internally, yes, but users can wrap anything they like in |
Why not? It seems to me that plenty of people use it that way, e.g. https://wiki.python.org/moin/UsingAssertionsEffectively
true... but then that would be an argument for using |
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.
regardless of the discussion above, content wise this is good to go in my opinion
a thought on |
this crossed my mind before. I guess I never thought it high priority. Not sure why, since trieste has already had problems with re asserts and exceptions, this Q&A might be of interest |
btw i don't have merge permissions. feel free to merge |
Use
tf.debugging
for assertions in space.py, so that error handling works intf.function
.Also changed an implementation while I was at it