bfrosik / data-quality

Other
5 stars 5 forks source link

Accomodate for multiple instruments at the same beamline #21

Closed decarlof closed 8 years ago

decarlof commented 8 years ago

Please review but don't accept yet, I still need to update the docs. Also there is a conflict since we both modified the wrapper functions, we can fix that manually.

bfrosik commented 8 years ago

Francesco, this look really good. I like the new dquality/check directory with the code for all cases. And then the demo that include 32id specifics. The 'wrapper' is not needed, since it is all in demo - looks nice and clean.

decarlof commented 8 years ago

I was also thinking to remove from the functions in check.py the parser since this is now also running in the corresponding demo function (= the parameter are already checked in the demo function so there is no need to repeat the parsing again) unless you expect someone to directly call them from main.

bfrosik commented 8 years ago

Yes, I agree. I thought about the same, while writing the wrappers, but thought that both will be used. With the refactoring it seems redundant.

decarlof commented 8 years ago

@bfrosik ok docs now builds again, I removed the redundant parsers and added a 'default' instrument folder that will be used if a station has only one instrument and/or does not pass the 'instrument' parameter in the function call; still I need to figure out how to make the parser work if a parameter is missing ...

decarlof commented 8 years ago

@bfrosik compiles and runs without errors but logs complains does not find files:

2016-06-14T19:32:08-0500: ERROR: dquality.pv: configuration error: pv_file is not configured 2016-06-14T19:32:13-0500: ERROR: dquality.pv: configuration error: pvs is not configured

etc.

I think I need a better understanding of what the get_file function in utilities does with the statement file = conf[config_name].

decarlof commented 8 years ago

@bfrosik thank you for the dictionary tips. All works now, please have a look. Let me know when you plan to merge, I have a quick solution for the merge conflict.

bfrosik commented 8 years ago

great, I will merge

bfrosik commented 8 years ago

You did a great work Francesco with reorganizing. What would be the merge tip?