Skip to content

Conversation

@dkazanc
Copy link
Collaborator

@dkazanc dkazanc commented Oct 2, 2024

fixes #157 #160 and #163

@dkazanc dkazanc changed the title removes dev build and corrects the rescaler distortion correction preview refactoring Oct 7, 2024
Copy link
Contributor

@yousefmoazzam yousefmoazzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

dkazanc commented Oct 8, 2024

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
Copy link
Contributor

yousefmoazzam commented Oct 8, 2024

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
Copy link
Collaborator Author

dkazanc commented Oct 8, 2024

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.

Copy link
Contributor

@yousefmoazzam yousefmoazzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants