What to do with reco.conf?

[tfarago]

Currently the reco.conf causes more harm than good because it's silently loaded and the user cannot override all the options, like originally intended, e.g. absorptivity. Before we start a long discussion, does actually anyone use that file @matze @rshkarin @ashkarin @milkdrop @lucito?

[matze]

Hmm, at least Francesco left it in his fork because he really liked that re-opening the GUI in a directory have all the parameters load again. But maybe it's better to make this more self-contained and provide a project file that can be load by opening it. But in fact, I don't know what's best …

[tfarago]

Or maybe there could be a switch which would be off by default, like --use-file-conf. Let's wait what the others think as well.

[sgasilov]

How about passing parameters everywhere in a more bulky, but more explicit way than currently used default': False and 'action': 'store_true' fancy functionality of the python's argparser? That is --absorptivity True/False (and all other parameter where necessary)? In this way you can keep reco.conf (which is a good thing by the way) and make sure that calls from the command line are executed with new parameters whenever desired, rather than those stored in the reco.conf?

[matze]

I gave this whole mess a little bit of thought. The current implementation is plagued by some weird decisions and a system that worked well in the past but has grown a little too much.

From my point of view the main problem at the moment is the arbitrariness of the sections which are supposed to structure the config file. At the beginning the idea was to have some kind of namespacing but since all argument names must be unique for a given command anyway, the current mapping (not the split though) is pointless. Moreover, we are leaking implementation detail to the user. Not nice. So, my first suggestion would be to build and write those sections for commands only. So instead of [general], [reading] etc. there would be sections for [reco], [perf] etc.

The second problem I see and which will only get worse with the universal reconstruction is the decision to make different reconstruction algorithms an option to a single command. So, my second suggestion would be to have a second-level command specifying the actual algorithm with all its parameters. So instead of

tofu reco --method fbp --args-for-fbp

there would be

tofu reco fbp --args-for-fbp

Third, there are no unit tests for this kind of stuff. So my last suggestions is, that there should be some :smile:

If no one disagrees, I will try to untangle the current mess and make it more straightforward for both users and developers.

[tfarago]

there would be sections for [reco], [perf] etc.

If we do it like this we will have a lot of duplicated code, e.g. plenty of parameters are the same for lamino and tomo.

The second problem I see and which will only get worse with the universal reconstruction is the decision to make different reconstruction algorithms an option to a single command.

This is not true. the tofu reco will get parameters from the user and run an optimized filtered back projection for the current geometry, e.g. parallel beam -> no feldkamp filtering. But by no means will it use anything else than FBP and it's corresponding ufo task (I will match the names in ufo and tofu in the squashing process).

If no one disagrees, I will try to untangle the current mess and make it more straightforward for both users and developers.

Please don't do it now! It will make my life very difficult, on the top of that the new code is almost finished, so let's merge that and do the make-up afterward.

My last cent here is that I completely agree with you about that it's messy now, we talked about that. We also talked about replacing tofu with something much more generic, like ufo-launch but maybe a bit more versatile. I think that tofu is very rigid and in the future we will stumble upon the same problems over and over again unless we come up with a good compromise between preconfigured task for simplicity and graph-creation flexibility for easy adjustment of existing preconfigured or completely new tasks.

[tfarago]

As a consequence, I would redesign the complete tofu. If we hit the sweet spot between simple usage and flexibility it will be a matter of one day to reimplemend all existing subcommands. Moreover, we will finally be able to have a generic GUI creator! I will be more than happy to help or even do most of the programming once we have solid and future-proof grounds.

[matze]

If we do it like this we will have a lot of duplicated code, e.g. plenty of parameters are the same for lamino and tomo.

Why would I reconstruct a tomographic dataset with lamino backprojection? Or vice versa? And frankly speaking, does it matter if it is saved multiple times? The problem is that the section as they are now just cause troubles. On the other hand, if all parameters belonging to a command are stored in the respected section it will be easier to understand.

By the way, I am not arguing against decoupling the definition of options. Re-using option definitions for different commands is definitely on my list.

This is not true.

I think you are not understanding what I am saying. At the moment, we have two (or three in the future) commands that trigger a reconstruction of some kind. On top of that there is an option --method for the tomo command that selects a particular tomographic reconstruction algorithm. This is just plain confusing.

Please don't do it now! It will make my life very difficult, on the top of that the new code is almost finished, so let's merge that and do the make-up afterward.

I am thinking about a prototype first without touching any code. No worries.

As a consequence, I would redesign the complete tofu.

In principle, I agree. But as far as I can see, the cost/benefit ratio is be that high at the moment because we restrict us anyway to a couple of standard procedures. And I think a certain amount of rigidness and restriction is not the worst that could happen, at least it lets one reason about. However, I won't stop you building the generic über-launch :-)

[tfarago]

Why would I reconstruct a tomographic dataset with lamino backprojection?

You have a tomographic data set and figure out you were misaligned, then you need a more generic algorithm which can deal with such things. It happens.

By the way, I am not arguing against decoupling the definition of options. Re-using option definitions for different commands is definitely on my list.

This is nice music to my ears.

I won't stop you building the generic über-launch :-)

OK, once the new code is in place I will start with this. BTW you already played with some templating if I remember correctly.

[tfarago]

I think you are not understanding what I am saying. At the moment, we have two (or three in the future) commands that trigger a reconstruction of some kind. On top of that there is an option --method for the tomo command that selects a particular tomographic reconstruction algorithm. This is just plain confusing.

I should have read more carefully... Yep that's a good idea with the second subcommand.

[tfarago]

Just to dump this somewhere so it's not forgotten. I can see the new tofu in a way which would encapsulate a task in some lightweight python class. That way, connections will be easy to make, GUIs will become trivial and we can even use third party packages like VisTrails.

Second thing is, if there is a python wrapper it will be possible for the users to add tasks written in python, which will be extremely useful for trying out new stuff, even for us I think. I think there were already a couple of people asking about this.

[sgasilov]

Hi, I have only glanced at your exchange of messages, but I'm already scared. Would you perhaps consider keeping current "stable" version of tofu as is - with a rigid structure, but that works good for those several essential operation which are used in 95% of the cases (namely flatcorrect or flatcorrect+TIE then tomo). And develop flexible launch module as a separate project? At least, please, do bring the Paganin/TIE phase retrieval question to a conclusion (whether it will be in preprocess.py or phase-retrieval-task.cpp) so I can grab a stable release, and then start working on the architecture of the tofu ))

Have an enjoyable weekend, S.

To illustrate what exactly am I scared of. The output below produced by the latest ufo-kit. Preprocess worked perfectly fine before Christmas:

tofu preprocess --projections apple_1_5_mm_Al/tomo_00016_02000.tif --flats apple_1_5_mm_Al_flat/ --darks apple_1_5_mm_Al_dark/ --output test_prepro.tif –fix-nan-and-inf --energy 18 --propagation-distance 0.2 --pixel-size 6.5e-6 --regularization-rate 2.5 --retrieval-padded-height 1024 --retrieval-padded-width 4096 --verbose DEBUG: General DEBUG: config reco.conf DEBUG: log - DEBUG: output test_prepro.tif DEBUG: output_bitdepth 32 DEBUG: output_maximum - DEBUG: output_minimum - DEBUG: verbose True DEBUG: width - DEBUG: Input DEBUG: bitdepth 32 DEBUG: height - DEBUG: number - DEBUG: resize - DEBUG: retries 0 DEBUG: retry_timeout 0 DEBUG: start 0 DEBUG: step 1 DEBUG: y 0 DEBUG: y_step 1 DEBUG: Flat field correction DEBUG: absorptivity False DEBUG: dark_scale 1 DEBUG: darks apple_1_5_mm_Al_dark/ DEBUG: fix_nan_and_inf False DEBUG: flats apple_1_5_mm_Al_flat/ DEBUG: flats2 - DEBUG: projections apple_1_5_mm_Al/tomo_00016_02000.tif DEBUG: reduction_mode Average DEBUG: Phase retrieval DEBUG: energy 18.0 DEBUG: pixel_size 6.5e-06 DEBUG: propagation_distance 0.2 DEBUG: regularization_rate 2.5 DEBUG: retrieval_method tie DEBUG: retrieval_padded_height 1024 DEBUG: retrieval_padded_width 4096 DEBUG: retrieval_padding_mode clamp_to_edge DEBUG: thresholding_rate 0.01 DEBUG: General reconstruction DEBUG: projection_filter ramp-fromreal DEBUG: projection_padding_mode clamp_to_edge DEBUG: Preprocess DEBUG: projection_filter ramp-fromreal DEBUG: projection_filter_scale 1.0 DEBUG: projection_padding_mode clamp_to_edge DEBUG: transpose_input False DEBUG: Writing output to test_prepro.tif DEBUG: Image width x height: 2048 x 648 DEBUG: Setting read:height to 648 DEBUG: Setting read:retries to 0 DEBUG: Setting read:retry_timeout to 0 DEBUG: Setting read:start to 0 DEBUG: Setting read:step to 1 DEBUG: Setting read:y to 0 DEBUG: Setting read:y_step to 1 DEBUG: Setting read:height to 648 DEBUG: Setting read:y to 0 DEBUG: Setting read:y_step to 1 DEBUG: Setting read:height to 648 DEBUG: Setting read:y to 0 DEBUG: Setting read:y_step to 1 DEBUG: Doing flat field correction using reduction mode `average' DEBUG: Creating phase retrieval pipeline DEBUG: Phase retrieval padding: 2048x648 -> 4096x1024 /usr/lib/python2.7/site-packages/tofu-0.9-py2.7.egg/tofu/preprocess.py:316: Warning: invalid unclassed poi nter in cast to 'UfoNode' /usr/lib/python2.7/site-packages/tofu-0.9-py2.7.egg/tofu/preprocess.py:316: Warning: invalid unclassed poi nter in cast to 'UfoTaskNode'

(process:55469): Ufo-CRITICAL **: ufo_task_node_set_partition: assertion 'UFO_IS_TASK_NODE (node)' failed /usr/lib/python2.7/site-packages/tofu-0.9-py2.7.egg/tofu/preprocess.py:316: Warning: invalid unclassed poi nter in cast to 'UfoTask' /usr/lib/python2.7/site-packages/tofu-0.9-py2.7.egg/tofu/preprocess.py:316: Warning: g_type_interface_peek : assertion 'instance_class != NULL' failed Segmentation fault (core dumped)

[matze]

Would you perhaps consider keeping current "stable" version of tofu as is - with a rigid structure, but that works good for those several essential operation which are used in 95% of the cases

That was my intention. I only wanted to fix the internal structure of the parameter/option handling and not change the interface to the user because I know the pain of adapting to new stuff all the time. Don't worry ;-)

[tfarago]

Hmm, I also get the segfault, did you change anything regarding padding and cropping in the filters Matthias? It seems it comes from there. If you don't have time I will investigate this further later. @milkdrop thanks for pointing this out.

[matze]

I see it as well, but it has nothing to do with those filters (why would I change them) but somewhere in the core. I will bisect the problem.

[matze]

Hmm, it's this change but for some reason I cannot reproduce this with ufo-launch, it does not crash and I get correct results. That will be some tough investigation.

[sgasilov]

I can also confirm that both padding and phase-retrieval are both working with ufo-launch. Good luck with the troubleshooting )

[matze]

Can you please check again the issue with the preprocess command? I fixed it on my machine but who knows what else broke. If everything is fine I will release version 0.15.1.

[sgasilov]

Can confirm that tofu preprocess with flat-correct and phase-retrieval pipelines works for me now. Got my version today: filters: fdc3b11 opencl: add diff kernel ufo-core 278b6b8 travis: do install libzmq3 tofu> a0a9da3 Do flat field correction before center estimation

[tfarago]

Is this issue still an issue or can I close it as well?