DiamondLightSource / DiamondLightSource/httomo

Methods without a `side_outputs` field may be unnecessarily included in `method_id_map`

Open
#475 0 comments 0 reactions 0 assignees View on GitHub
question refactor
Dominant language
Python
Stars
10
Forks
5
Avg merge
5d 17h
Merged PRs (30d)
1

Description

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: https://github.com/DiamondLightSource/httomo/blob/7842bae27c1777545279aa36e8c10d9c4eeb489f/httomo/ui_layer.py#L64-L72

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
https://github.com/DiamondLightSource/httomo/blob/7842bae27c1777545279aa36e8c10d9c4eeb489f/httomo/ui_layer.py#L64-L72

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

https://github.com/DiamondLightSource/httomo/blob/7842bae27c1777545279aa36e8c10d9c4eeb489f/httomo/ui_layer.py#L110

and some dumb printing in the loop in `UiLayer.build_pipeline()` confirms this:
```
i = 1, method_id_map is now {'centering':
}
i = 2, method_id_map is now {'centering':
, 'task_2':
}
i = 3, method_id_map is now {'centering':
, 'task_2':
, 'task_3':
}
i = 4, method_id_map is now {'centering':
, 'task_2':
, 'task_3':
, 'task_4':
}
...
```

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`?

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.