Closed tom-andersson closed 2 years ago
@wesselb, it looks like the test failed due to a numerical error, is that correct? Should we restart them?
The Python3.7 tests took over an hour. Is this my fault by any chance? Have the lines I've added in https://github.com/wesselb/neuralprocesses/pull/3/commits/d451018d76da991ed76a99cb3097352e8ebc50ef increased the number of construct_convgnp
tests by a factor of four? Are these ..._learnable
kwarg tests overkill and would a simpler one-off test be preferable?
Hey @tom-andersson! Thank you for putting this together!! This looks great—I have no comments. IMO, can be merged straight away. :)
@wesselb, it looks like the test failed due to a numerical error, is that correct? Should we restart them?
I think they indeed failed due to a numerical error. The tests are not terribly stable at the moment, which is annoying and should be improved. All other tests passed successfully, so I think we can just ignore the failed one.
The Python3.7 tests took over an hour. Is this my fault by any chance? Have the lines I've added in https://github.com/wesselb/neuralprocesses/commit/d451018d76da991ed76a99cb3097352e8ebc50ef increased the number of construct_convgnp tests by a factor of four? Are these ..._learnable kwarg tests overkill and would a simpler one-off test be preferable?
The tests mostly take ages due to the tests for the EEG data. This is something that should be fixed—an hour is too long. The added keyword arugments have indeed increased the number of tests by a factor four, but just for the ConvGNP. But those tests are fairly cheap, so I think that should be fine for now.
In conclusion, I think everything looks excellent! Will be merging this shortly. :)
This PR addresses a problem I've experienced, whereby learnable SetConv length scales in the ConvGNP arch lead to encoder length scales shrinking to the point of producing checkerboard-like artifacts in the discretised density channel output by SetConv layers. This has the effect of injecting noise into the model as training progresses, leading to artifacts in the model predictions. I've addressed this by adding a parameter to hold the length scales fixed at their initialisation values:
construct_convgnp
, with the encoder and decoders parameterised separatelylearnable
args toTrue
construct_convgnp
unit tests. Are there any more tests I should add? I ran the tests locally and I think the only errors were due to numerical issues (eg Cholesky decomp). I also killed the tests at 98% completion, because thetest_predprey
andtest_eeg
tests were taking too long on my laptop.construct_convgnp
, as discussed with @wesselb