Libertem: RAW: TypeError when omitting detector_size

[sk1p]

To reproduce (file doesn't need to exist):

ctx.load("raw", path="/tmp/meh/meh.raw", dtype="fliat32", scan_size=(256, 256))

result:

TypeError: 'NoneType' object is not iterable

Should instead throw an understandable error message.

[twentyse7en]

when path parameter was missing :

ctx.load("raw", dtype="fliat32", scan_size=(256, 256), detector_size=(128, 128))

error :

TypeError: __init__() missing 1 required positional argument: 'path'


error message like that is required right ?

[twentyse7en]

Can you tag difficulty level for issues that would be helpful.

[sk1p]

error message like that is required right ?

Yes, that would be great. Most importantly, the backwards-compatibility logic, handling the old parameters, needs to keep working.

Can you tag difficulty level for issues that would be helpful.

That's a good point; @uellue we should go through the issues on Monday.

This issue should be an easy fix; note that as opposed to the other PRs, a regression test for this will be needed.

[twentyse7en]

regression test ? what does that means.

[sk1p]

regression test ? what does that means.

Sorry I can't give a full answer today (out of office..) but reading the following wikipedia pages should help:

https://en.wikipedia.org/wiki/Test_automation
https://en.wikipedia.org/wiki/Regression_testing
https://en.wikipedia.org/wiki/Test-driven_development
https://en.wikipedia.org/wiki/Continuous_integration

For concrete example tests, see

https://github.com/LiberTEM/LiberTEM/tree/master/tests
https://github.com/LiberTEM/LiberTEM/blob/master/tests/io/test_raw.py

We are using pytest for running the tests, see https://docs.pytest.org/en/latest/ for documentation

Good first step is getting the tests to run on your system, for that see:

https://libertem.github.io/LiberTEM/contributing.html#running-the-tests

[Akhileshpm]

@twentyse7en Regression testing (rarely non-regression testing[1]) is re-running functional and non-functional tests to ensure that previously developed and tested software still performs after a change.[2] If not, that would be called a regression.
This is what I got from wiki. Hope you will understand this.

[sk1p]

This is what I got from wiki

:+1: - this can also include a) writing a test case that fails in the current version of the software and shows that there is a problem, b) actually fixing the problem, c) re-running the tests to make sure that the issue is fixed. By adding this test case, you make sure that this bug is never re-introduced in the future.

[twentyse7en]

is detector_size a optional parameter or must have ?

[twentyse7en]

In raw.py,
Before :
def __init__(self, path, scan_size, dtype, detector_size=None, enable_direct=False, detector_size_raw=None, crop_detector_to=None, tileshape=None):

After :
def __init__(self, path, scan_size, dtype, detector_size, enable_direct=False, detector_size_raw=None, crop_detector_to=None, tileshape=None):

removed None for detector_size.
Now it gives the error TypeError: __init__() missing 1 required positional argument: 'detector_size'
Run tox, nothing failed.

Is this correct direction ?

[sk1p]

@twentyse7en thank you for looking into this! This is almost right: not passing in detector_size should give this or a similar error. But: we still want to support an old way of opening raw file:

ctx.load("raw", path="/tmp/meh/meh.raw", dtype="float32", scan_size=(256, 256), detector_size_raw=(128,128), crop_detector_to=(128, 128))

Note that instead of detector_size we passed in two parameters detector_size_raw and crop_detector_to.

Run tox, nothing failed

You discovered a missing test! It would be good to have tests for at least the following cases:

1) Opening a raw file the correct way, with detector_size (this already exists)
2) As described in the original issue: the test case should check that a good error message is raised for the missing detector_size parameter
3) Opening a raw file the old way, with detector_size_raw and crop_detector_to

If you would like to have a more detailed look, you can also check the test coverage to make sure all branches are run properly.

We should remove these old parameters for 0.6 - the deprecation warnings should be updated to include this. See this for an example.

[twentyse7en]

Got it :+1:

[twentyse7en]

Could you provide me some insight about capturing DeprecationWarning.

[sk1p]

Could you provide me some insight about capturing DeprecationWarning.

You can use warnings.catch_warning, like in this testcase. See also the official documentation