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

Revaluate whether StandardLoaderWrapper is needed or not #504

Open
yousefmoazzam opened this issue Oct 21, 2024 · 0 comments
Open

Revaluate whether StandardLoaderWrapper is needed or not #504

yousefmoazzam opened this issue Oct 21, 2024 · 0 comments

Comments

@yousefmoazzam
Copy link
Collaborator

TL;DR

  • the runner wants to be able to get a loader generically, so it relies on any implementor of LoaderInterface to get one
  • the StandardLoaderWrapper class implements LoaderInterface, so the runner uses StandardLoaderWrapper to get a loader
  • the StandardLoaderWrapper class wraps the StandardTomoLoader class, so then the latter can be used by the runner
  • the StandardTomoLoader class can be very slightly modified (adding three property-getters, detector_x, detector_y, angles_total) to implement LoaderInterface, to avoid the extra layer of StandardLoaderWrapper
  • remove the make_data_source() method on the LoaderIntarface protocol (which then means that StanardTomoLoader now would implement LoaderInterface)
  • then, make the loader factory function make_loader() return an instance of StandardTomoLoader instead of StandardLoaderWrapper

Details

The Pipeline object represents the loader and methods in the pipeline. In particular, for the loader, it requires anything that implements the LoaderInterface protocol:

def __init__(self, loader: LoaderInterface, methods: List[MethodWrapper]):
self._methods = methods
self._loader = loader

When the UI layer creates a Pipeline object, it creates an implementor of LoaderInterface by using the make_loader() function (which could be viewed as a factory function for implementors of LoaderInterface):

httomo/httomo/ui_layer.py

Lines 143 to 159 in f6bb8ac

loader = make_loader(
repo=self.repo,
module_path=task_conf["module_path"],
method_name=task_conf["method"],
in_file=Path(in_file),
data_path=data_path,
image_key_path=image_key_path,
angles=angles,
darks=DarksFlatsFileConfig(
file=darks_file, data_path=darks_path, image_key_path=darks_image_key
),
flats=DarksFlatsFileConfig(
file=flats_file, data_path=flats_path, image_key_path=flats_image_key
),
preview=preview,
comm=self.comm,
)

The task runner then uses the make_data_source() method defined on the LoaderInterface protocol to generate an implementor of DataSetSource:

self.source = self.pipeline.loader.make_data_source(padding=loader_padding)

Now, the way that StandardLoaderWrapper implements LoaderInterface is mostly by using the StandardTomoLoader.global_shape getter:

def make_data_source(self, padding: Tuple[int, int] = (0, 0)) -> DataSetSource:
assert self.pattern in [Pattern.sinogram, Pattern.projection]
loader = StandardTomoLoader(
in_file=self.in_file,
data_path=self.data_path,
image_key_path=self.image_key_path,
darks=self.darks,
flats=self.flats,
angles=self.angles,
preview_config=self.preview,
slicing_dim=1 if self.pattern == Pattern.sinogram else 0,
comm=self.comm,
padding=padding,
)
(self._angles_total, self._detector_y, self._detector_x) = loader.global_shape
return loader
@property
def detector_x(self) -> int:
return self._detector_x
@property
def detector_y(self) -> int:
return self._detector_y
@property
def angles_total(self) -> int:
return self._angles_total

Note that the three property getters:

  • detector_x
  • detector_y
  • angles_total

are simply using StandardTomoLoader.global_shape, and that the make_data_source() method is implemented simply by creating a StandardTomoLoader instance.

What this means is that:

  • if LoaderInterface only required the three property getters detector_x, detector_y, angles_total
  • then StandardTomoLoader could easily be changed to implement LoaderInterface
  • and the runner would be able to get access to an implementor of both LoaderInterface and DataSetSource from having an instance of StandardTomoLoader

Lastly, the make_loader() factory function could then simply create instances that implement both:

  • LoaderInterface
  • DataSetSource

and the runner would be able to get a data source without needing to use an intermediate "loader wrapper class" to generate a data source.

I think this could possibly make the "way" that the runner gets access to a data source simpler, and easier to understand, by getting rid of a layer in the generation of data source.

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

No branches or pull requests

1 participant