distortion correction preview refactoring

[dkazanc]

fixes #157 #160 and #163

[yousefmoazzam]

Changes to the distortion correction method looks good to me 🙂 Though I don't understand why there's a random change to the nightly dev workflow file in this PR?

It's a bit muddled to have two totally unrelated issues tackled in the same PR, but if that must be done, could the "backup" file be removed completely instead of just being renamed? The backup of the file will exist in past states of the repo via version control, so removal of the file is cheap to do (ie, it's easy to find the backup file), without needing to keep the backup file under version control.

[dkazanc]

OK, I got rid of the dev CI backup and also changed tuples for shift and step into lists. I believe there is an issue to cast a tuple correctly in httomo resulting in a something like this after yaml generator:

    pad: !!python/tuple
    - 0
    - 0

There is quite a lot of problems like this with Tomopy functions.
Feel free to add preview string to parameters name if you think it will be a better way to search for them in httomo.

[yousefmoazzam]

Oh right, with the mention of the parsing YAML into a python tuple, are we planning on having users entering the values for shift and step in the YAML directly?

Meaning, are we going to have something like the following in the YAML for the distortion correction:

- method: standard_tomo
  module_path: httomo.data.hdf.loaders
  parameters:
    name: tomo
    data_path: entry1/tomo_entry/data/data
    image_key_path: entry1/tomo_entry/instrument/detector/image_key
    rotation_angles:
      data_path: /entry1/tomo_entry/data/rotation_angle
  preview:
    detector_y:
      start: DET_Y_START
      step: DET_Y_STEP
    detector_x:
      start: DET_X_START
      step: DET_X_STEP
.
.
.
- method: distortion_correction_proj_discorpy
  module_path: httomolibgpu.prep.alignment
  parameters:
    metadata_path: /some/path
    shift: [DET_X_START, DET_Y_START]
    step: [DET_X_STEP, DET_Y_STEP]
    order: 1
    mode: reflect

where the users have some value for DET_Y_START, DET_X_START, DET_Y_STEP, and DET_X_STEP that they put in two places?

The reason I ask is because I think the YAML would need to be parsed into a tuple only if the YAML contains the actual values to go into the distortion correction method?

But then in order for the values for shift and step to be put into the YAML (to be parsed into a python tuple), the values need to be duplicated in the YAML config:

• once in the loader's preview
• once in the distortion correction's shift and step

Or have I misunderstood how this would work (which is very possible at this point...)?

[dkazanc]

I see, we're indeed would like to deal with those parameters behind the scenes in HTTomo, so there is no need for us to expose those parameters in YAML to users? Yes, it makes sense then. We've got glob_stats as a tuple as well, so I don't mind tuple or list, as it will be hidden anyway.

[yousefmoazzam]

Due to it still not being clear how we'll change parameters in the method to allow httomo to pass the relevant info from the loader's preview to the method, I won't make any changes here yet, just some comments about types.

[yousefmoazzam]

If we go with a list, I think the correct type would be List[int]. Currently, the type List[Union[int, int]] contains the redundant type Union[int, int] which means "an int or an int". Altogether, the type List[Union[int, int]] means "a list of values with type int or int", which I imagine is not what was intended, and the intention more likely was "a list of values of type int".

If we go with a tuple, the type should be Tuple[int, int].

[yousefmoazzam]

Similar to the other comment, a note about types.

If we go with a list, the type should be List[int] (no optional).

If we go with a tuple, the type should be Tuple[int, int] (no optional)

[yousefmoazzam]

Two things to note:

1. Union isn't needed, regardless of if we choose a list or tuple for the shift and step params
2. remove Tuple import if we go with a list