Checking updated PR...

No PEP8 issues.

Comment last updated on August 29, 2018 at 11:28 Hours UTC

I don’t understand the Travis failure. :(

Seems to be this issue. Can you try older versions of pytest? Looks like we need to blacklist a few.

I can confirm that pytest 3.6.1 works on my laptop until it hits odl/test/tomo/backends/astra_cuda_test.py .[1] 5393 abort (core dumped) pytest --pep8 (my computer does not have an Nvidia GPU), whereas pytest 3.7.1 breaks with the recursion problem.

Edit: On my workstation, pytest 3.6.2 works fine (and it has got an Nvidia GPU), whereas pytest 3.7.1 breaks again.

Sounds very much like this issue:

astra-toolbox/astra-toolbox#109

Try updating astra.

Regarding the ASTRA issue: to me it looks like #684, which should have been fixed in #1376. But for the fix in #1376 to work, one need the ASTRA update that corresponds to the bug report astra-toolbox/astra-toolbox#109 that @adler-j mentioned. However, I think that this is still not in the latest release of ASTRA.

You can check the ASTRA thing with a dev build from @wjp's conda channel:

conda install -c wjpalenstijn astra-toolbox=1.9.0dev

I'll try this later, since the pytest upgrade breaks my tests anyway. ;-)

Regarding the pytest recursion issue: It does not occur in version 3.6.4 (I tested in on my workstation), which is the newest version available via conda previous to 3.7.1, and according to the link @kohr-h posted, it should be fixed in version 3.7.2.

Edit: Corrected the version where the bug should be fixed.

Thanks for investigating, @sbanert!

should be fixed in version 3.7.1

I guess you mean 3.7.2 (the upcoming release).

In the meanwhile, we should blacklist the affected versions 3.7.0 and 3.7.1. Not that it's a terribly important core dependency, but the deps that are pulled in by default should work. There are two places where we have control over the version.

Here:

and here:

For the syntax to blacklist versions you can check the numpy line in the requirements.txt file.

However, I suggest that we deal with it in a separate issue, since I think we might want to bail out or at least warn in odl.test() as well.

Regarding the ASTRA issue: After applying @kohr-h’s upgrade suggestion, the tests do no longer abort. However, I get 12 failures like this:

__________________ test_projector[ geom='cone2d' - impl='astra_cpu' - angles='random' - in-place ] __________________

projector = RayTransform: uniform_discr([-20., -20.], [ 20.,  20.], (100, 100), dtype='float32') -> DiscreteLp(    FunctionSpace(I...
        [-29.7, -29.1, -28.5, ...,  28.5,  29.1,  29.7]
    ),
    rn((100, 100), dtype='float32', weighting=0.03651))
in_place = True

    def test_projector(projector, in_place):
        """Test Ray transform forward projection."""
    
        # TODO: this needs to be improved
        # Accept 10% errors
        places = 1
    
        # Create Shepp-Logan phantom
        vol = projector.domain.one()
    
        # Calculate projection
        if in_place:
            proj = projector.range.zero()
            projector(vol, out=proj)
        else:
            proj = projector(vol)
    
        # We expect maximum value to be along diagonal
        expected_max = projector.domain.partition.extent[0] * np.sqrt(2)
>       assert almost_equal(proj.ufuncs.max(), expected_max, places=places)
E       AssertionError: assert False
E        +  where False = almost_equal(33.670719, 56.568542494923804, places=1)
E        +    where 33.670719 = <bound method TensorSpaceUfuncs.max of <odl.util.ufuncs.TensorSpaceUfuncs object at 0x7ff89fbf6518>>()
E        +      where <bound method TensorSpaceUfuncs.max of <odl.util.ufuncs.TensorSpaceUfuncs object at 0x7ff89fbf6518>> = <odl.util.ufuncs.TensorSpaceUfuncs object at 0x7ff89fbf6518>.max
E        +        where <odl.util.ufuncs.TensorSpaceUfuncs object at 0x7ff89fbf6518> = DiscreteLp(    FunctionSpace(IntervalProd([  0.1072, -30.    ], [  6.1924,  30.    ]), out_dtype='float32'),\n    nonun...  11.42340565],\n     [ 20.49684906,  24.00613022,  24.0072403 , ...,  14.09786224,\n       12.93267536,  11.78796482]]\n).ufuncs

odl/test/tomo/operators/ray_trafo_test.py:217: AssertionError

Might these errors be due to #1415?

Might these errors be due to #1415?

Not exactly that, because we only look at forward projection here and check that we get close to the maximum ray intersection length. The change only affects the backprojection.
However, that's not to say that no other scaling factors changed 😉

Can I ignore the Python 2 failure? Otherwise, I think that I addressed all remarks.

Can I ignore the Python 2 failure? Otherwise, I think that I addressed all remarks.

Yeah that's just some annoying conda issue, ignore it. I'll have a look at the code.

Just a general comment w.r.t the git flow here. The commit messages here are typically of this type: "MAINT: Remove algorithm parameters and use keyword arguments."

While this is locally informative (e.g. me following this PR, its obvious what you mean), being locally informative is not really what we're after down the line. I'd personally prefer if there was at least a hint as to where the change was done, e.g. "MAINT: Remove algorithm parameters and use kwargs in adupdates example." That way, one year from now when you're confused as to why things are as they are, it's a lot easier to scroll through the commits.

Another option (actually the best option if you had infinite time available) is that you use these "local" commit messages during development, and then before the branch is merged, you rebase the branch and merge commits appropriately, also updating the commit messages. For example, this whole PR could quite frankly be squashed into a single commit with all the intermediate commit messages ("MAINT: Add relative instead of absolute noise.") removed.

I can do, as you suggested, an interactive rebase before we merge. My reason is basically that it is easier to see what I changed after the reviews and that it’s easier to revert specific changes if someone disagrees with them.

That's a very reasonable general workflow @sbanert.
In this case, though, the PR was already approved, so the only thing left to do was "whatever you want done before merge". When that's done I'd just merge it after having a last glance.

So please go ahead and take any final steps left to do, ping me, and I'll merge it.