Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Methods without a side_outputs field may be unnecessarily included in method_id_map #475

Open
yousefmoazzam opened this issue Oct 10, 2024 · 0 comments
Labels
question Further information is requested refactor

Comments

@yousefmoazzam
Copy link
Collaborator

yousefmoazzam commented Oct 10, 2024

A variable methods_id_map is defined in the UI layer when there is iteration over the methods in the pipeline config, getting ready to build the Pipeline object:

method_id_map: Dict[str, MethodWrapper] = dict()
for i, task_conf in enumerate(self.PipelineStageConfig[1:]):
parameters = task_conf.get("parameters", dict())
valid_refs = get_valid_ref_str(parameters)
update_side_output_references(valid_refs, parameters, method_id_map)
self._append_methods_list(
i, task_conf, methods_list, parameters, method_id_map
)

Based on how this variable is used by update_side_output_references() to map:

  • a reference to a method X with an id field, from a method Y (ie, recon referencing a centering method with id: centering)
  • to the method wrapper with that ID
    method_id_map: Dict[str, MethodWrapper] = dict()
    for i, task_conf in enumerate(self.PipelineStageConfig[1:]):
    parameters = task_conf.get("parameters", dict())
    valid_refs = get_valid_ref_str(parameters)
    update_side_output_references(valid_refs, parameters, method_id_map)
    self._append_methods_list(
    i, task_conf, methods_list, parameters, method_id_map
    )

it would appear that this methods_id_map is solely for dealing with side outputs.

However, the methods_id_map includes both:

  • methods with an id field
  • methods without and id field

method_id_map[method.task_id] = method

and some dumb printing in the loop in UiLayer.build_pipeline() confirms this:

i = 1, method_id_map is now {'centering':
<httomo.method_wrappers.rotation.RotationWrapper object at 0x7f3d48321f60>}
i = 2, method_id_map is now {'centering':
<httomo.method_wrappers.rotation.RotationWrapper object at 0x7f3d48321f60>, 'task_2':
<httomo.method_wrappers.dezinging.DezingingWrapper object at 0x7f3d41d8c220>}
i = 3, method_id_map is now {'centering':
<httomo.method_wrappers.rotation.RotationWrapper object at 0x7f3d48321f60>, 'task_2':
<httomo.method_wrappers.dezinging.DezingingWrapper object at 0x7f3d41d8c220>, 'task_3':
<httomo.method_wrappers.generic.GenericMethodWrapper object at 0x7f3d41d8e110>}
i = 4, method_id_map is now {'centering':
<httomo.method_wrappers.rotation.RotationWrapper object at 0x7f3d48321f60>, 'task_2':
<httomo.method_wrappers.dezinging.DezingingWrapper object at 0x7f3d41d8c220>, 'task_3':
<httomo.method_wrappers.generic.GenericMethodWrapper object at 0x7f3d41d8e110>, 'task_4':
<httomo.method_wrappers.generic.GenericMethodWrapper object at 0x7f3d41d8c190>}
...

Question: given that methods_id_map seems to only be used for side outputs, is there a particular reason why methods without an id field (which produce no side outputs, and thus have no reason to be referenced) are still added to methods_id_map?

@yousefmoazzam yousefmoazzam added question Further information is requested refactor labels Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested refactor
Projects
None yet
Development

No branches or pull requests

1 participant