Modified and improved Cirpad class

[alexmarie78]

Hello, Finally we have come to a definition that suits us well for the moment. I tried to cleaned up the commits into one. I guess that one closes PR #1113.

[vallsv]

Hi,

Here is some comments, about the style, i just checked it very fast

FIrst few comment which have to be fixed

• Module header should be like that: license, then module description ("""This module blabla"""), then __future__ (sometimes before the module description), then metadata like owner (__license__), then import libs. You can copy-paste an existing module.

Then comment about things which are not needed to be fixed

• I am not against more commits, especially commits for typo (code alignment, rewrite few lines for readability). It's more easy to check. For example changes on ImXPadS10 looks to be typo only, but as there is no dedicated commit for that, with a clean comment that nothing have change on this side, i have to read it very carefully.
• It would be better to avoid to change the license text. I mean it's a copy paste from another module, it should always be the same. If you use a tool to auto-indent, and if i also use a tool to auto-indent, we will spend time over and over.
• aliases should not be changed, it could be stored externally by user configuration files (backward compatibility). But as you own this unique detector, that's on your side.

[alexmarie78]

Thanks for reply. I'm on it about what have to be fixed. I will not squash commits together in the future to keep a sort of historic as you want. Changes as license text or on ImXPadS10 are the fact of Frederic. Thus I don't know what was the purpose of those.

[alexmarie78]

One thing that I forgot to ask : Are you dropping python2 in 0.18 release ? Because some tests don't pass caused by commands that only exist in python3.

[vallsv]

I don't know for Python2. It's usually not difficult to use compatible syntax, but it's time consuming yes. I would say, while CI uses Python2 we have to support it.

For the git history, i usually try to clean up it by reordering commits and merge some of them. It's not always easy nor possible. In my POV the history should have a meaning (a sequence of atomic changes which are always runnable and testable). To make the thing easy to review, and easy to bisect.

[alexmarie78]

I'm hitting an error while running run_tests.py that the get_config() of the cirpad causes. I'm returning an OrderedDict filled by calibs (every 6 parameters of every modules) so this is a list of 20 calibs, one calib is one item of the OrderedDict. Here it is the error :

detector = detectorClass(**config)
TypeError: __init__() got an unexpected keyword argument 'calib6'

[vallsv]

Yes sorry.

The constructor will take the result of get_config.

THen your class constructor have to support self.__class__(**self.get_config())

As calib6 is part of your dictionary, it became one of the mandatory field of your constructor.

[vallsv]

You can deal it with def __init__(**kwargs) as constructor.

[alexmarie78]

Thanks a lot, I dealt with all the errors thanks to you, but I have a failure in the openCL_tests that is not my fact, I think. Here it is :

FAIL: test_OpenCL (pyFAI.test.test_openCL.TestMask)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/experiences/diffabs/mariea/Github/pyFAI/pyFAI/build/lib.linux-x86_64-3.5/pyFAI/test/test_openCL.py", line 137, in test_OpenCL
    self.assertTrue(r < 6, "Rwp=%.3f for OpenCL histogram processing of %s" % (r, ds))
AssertionError: False is not true : Rwp=19.710 for OpenCL histogram processing of {'spline': None, 'poni': '/tmp/pyFAI_testdata_mariea/Pilatus1M.poni', 'img': '/tmp/pyFAI_testdata_mariea/Pilatus1M.edf'}

[vallsv]

• Restarting the tests could help.
• Else you can disable OpenCL with run_tests using --no-opencl (i guess this problem is not related to changes on your Cirpad).
• If there is a real issue with OpenCL on your computer you can open a new one.

[kif]

Would it be possible to perform the import of geometry into the method call ? I had many users who complained about the "cost" in number of imports of pyFAI.

Beside this, do you know named-tuples can be seen as dict ? For the serialization part it may be useful https://docs.python.org/2/library/collections.html#collections.somenamedtuple._asdict

[alexmarie78]

I think it is possible to do so about geometry into the method call. About the _asdict() method, can you tell me if it is used as you wanted to be in the get_config() method of the cirpad class?

[kif]

It looks OK ... do you still have work to do on it?

[alexmarie78]

We still have work to do on it: We will make a notebook to deal with the calibration. From the results of calibration, we may have some changes to the definition.

[kif]

Inactive for too long. Feel free to re-open when ready.