add unit tests for preprocess pipeline #289
Conversation
junchaoxia
force-pushed
the
preprocess_test
branch
from
11786ba to
2d7037b
Compare
junchaoxia
force-pushed
the
preprocess_test
branch
from
2d7037b to
be1b08e
Compare
|
@janden This PR have included the tox updates. |
Conflicts: src/aspire/image/__init__.py
|
My run on my laptop also passed all tests. I have asked [email protected] to enable the debug mode for our repo yesterday. I got reply back this morning and will try to do something today. |
|
Also, that grid_3d should be getting a dtype. We're sometimes using self.dtype, but most of those computations are still default (doubles) fwiw. That yields 5.14984130859375e-05 for the energy conservation delta, which is right near the cutoff. (please disregard the prior numbers they were the wrong test method). I think you can diagnose this a few different ways, but if you are doing debug mode that should be effective. Better we find it now than random users. |
|
here a short summary of travis-ci failure for py38.
Rotation 0 for py36: Rotation 0 from py 37: Rotation 0 from py38:
|
|
I don't think issue is with Finufft, because that would have the exactly same version of that package between Other packages, like Numpy and scipy are not the same version between those (stable, dev) variants, they will be upgraded I suggested above that changing the numpy/scipy versions changes the output. It is possible the base version of certain packages for py38 are not agreeable. @janden, if we want to pause apple picker I can look at this instead just to help get it out the door. |
|
In the new push, I have tried to print out the result after the nufft transformation. After that there is |
|
Ok I see. Yeah it might be the `centered_ifft2' then that's causing problems then. Do we know if it's calling the SciPy FFT or FFTW here?. @garrettwrong if you want to pause on apple picker and take a look at this, I think it would be very helpful. |
|
Sure thing, I'll pick it up on Monday. I'm pretty suspicious about versions of the numerical libraries because py38-dev seems okay. The only difference being pip upgrades... |
|
After noting the difference in BLAS I replaced the np.linalg.norm call in anorm with sqrt(sum(x*x)) for testing purposes (since we know norm and dot are sensitive from some other tests)... This changes the results enough to get under the cutoff that was failing. It is not clear to me if it is the final comparison norm or somewhere earlier in the procedure (though I should know that later). I noticed some of the grid calls might have the wrong dtype, but that is probably red herring. |
What I don't understand yet is why this particular unit test is the only one sufficiently debased. |
py36-stable py37-stable py38-stable
The strange thing for py38-dev is that it installed scipy-1.5.4 not scipy-1.4.0 that we ask, but the unit test were passed.
|
|
I'm having trouble understanding most of your comment. Some of it doesn't make much sense. This isn't strange because that is how the tox testing environments are designed to work, as we have discussed previously. The I described a source of the numerical issue already above. Under Python 3.8 on this platform the older (pinned) versions of numpy/scipy are not finding openblas, as we are in the other environments. This can easily be demonstrated. I also wrote a specific calculation that you can replace to exercise this (and pass the test..). A lot of people had similar problems with numpy/scipy when 3.8.0 was released. Most of the time developers just told people to upgrade as bug fixes rolled out in later versions.... which is an option... but I have not been able to find any compatibility matrices to cite a minimal upgrade. However, we already know the latest stable versions work fine.... There might be some simpler things to do. It would not surprise me if anything FFT returns different results using different numerical libraries. |
|
@garrettwrong For my first point I try to confirm that the images after nufft should the same for all python versions since the volume and rotation are identical and we are use the same version of nufft lib. Some numbers from the last row of py38-stable has slightly differences to that of py36 and py37. But probably it is OK. I did not follow the anorm and np.linalg.norm previously. But it does can generate accumulated error if we use large number of images and high resolution. In default, if we do not specify the |
hrrm, maybe diffs typical of different numerical libraries...
No, only
So all these tests might have implored us to catch a bug in the test code. That would be satisfying. FWIW, installing openblas to the system before the pip install does allow the |
|
So if I understand correctly, the issue seems to be that we're using a different BLAS implementation in |
|
Correct. As I understand it, the base Python 3.8 configuration is not installing openblas, but the other base python environments are; and this is based on various Python package defaults, not our code (unless we manually do so like I pasted above). The I don't know that not using openblas is directly an issue, so much as it has the effect of tickling that norm line. But generally yes on those three points. If taking a more targeted norm works, that would be a better fix. |
You mean looking at norms per image instead of overall norms? Then yes, I agree. |
|
@janden @garrettwrong The submitted job finished last night. Here is a short summary.
Downsample image 0 from py36-stable Downsample image 0 from py37-stable Downsample image 0 from py38-stable |
|
I think the best thing to do is to just fix the norm call as you have applied it. I like that the CI platform did not admit the test code and drove the issue to be addressed. I would prefer to leave the library situation as it was in |
|
@garrettwrong I committed the change of adding libopenblas-dev before seeing your comment. I will wait @janden and revert it if necessary. |
|
Great. Let's revert the library change (would be good to keep as close an environment as possible to what end users will see) and we should be good to go. |
junchaoxia
force-pushed
the
preprocess_test
branch
from
d24653f to
beaea3c
Compare
|
@janden Reset the commit to remove the change of adding openblas lib. |
Closing issue #234.