search

plenoptic-org / plenoptic

Visualize/test models for visual representation by synthesizing images.
https://plenoptic.readthedocs.io/en/latest/
MIT License
57 stars 9 forks source link

Move `DN_sigmas.npy` and `DN_filts.npy` to be accessed with data module? #263

Closed billbrod closed 3 weeks ago

billbrod commented 2 months ago

Related to #235 : we have two .npy files that currently live within the src/plenoptic/metric directory and are used by the normalized_laplacian_pyramid function, found within src/plenoptic/metric/perceptual_distance.py. That's the only place they're used, and we don't expect the user to interact with them directly. They are accessed by calling np.load(os.path.join(DIRNAME, 'DN_filts.npy')) and sigmas = np.load(os.path.join(DIRNAME, 'DN_sigmas.npy')), where DIRNAME is defined at the top of perceptual_distance.py as DIRNAME = os.path.dirname(__file__).

Is this fine, or should we move to handling them more similarly to the example images, using importlib. @NickleDave, pinging you since you wrote the initial issue.

NickleDave commented 2 months ago

I am not completely sure but in this case I think you still want to use importlib.resources, so you can be confident that you can get the file, even when your package is installed as a wheel, as opposed to installed in your dev environment or installed as an sdist.

The reason, IIUC, is that if your package is installed as a wheel, it's basically a zip file, and so the npy files will be "inside" the zip, and not have a path on the actual file system. This is why the importlib-resources docs talk about "The use of __file__ doesn’t work if your package lives inside a zip file, since in that case this code does not live on the file system.". See https://importlib-resources.readthedocs.io/en/latest/using.html#example

I am not sure if there's an easy way to test this -- build a wheel, install in a local venv, and then see if importing the perceptual_distance metric fails?

But I would do it using the importlib_resources.files function as Jonny showed in their example from #235

Note there's a separate as_file function to use if it turns out that numpy demands that the npy actually exist on the file system, that makes use of a TemporaryFile: https://importlib-resources.readthedocs.io/en/latest/using.html#file-system-or-zip-file

NickleDave commented 2 months ago

I don't think you need to move them to be accessed with the data module if they are only used with this class though -- if someone wants to see the values, they can access them as an attribute of the class or the module, right?

They are mainly for this class, right, so it makes sense to me to keep them there.

Sorry if that's all you were actually asking

billbrod commented 2 months ago

This is helpful, thanks! So I think we'll keep the files there, accessible as an attribute of the class, but make use of importlib.resources to find them (since we're currently using __file__).