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

Generalize (deprecate?) get_xyzt_units #1116

Open
effigies opened this issue Jun 16, 2022 · 2 comments
Open

Generalize (deprecate?) get_xyzt_units #1116

effigies opened this issue Jun 16, 2022 · 2 comments

Comments

@effigies
Copy link
Member

Nifti1Header.get_xyzt_units is entirely based on the bitfield as it is implemented in nifti1.h. Attempting to generalize it to Minc (#1098) requires reducing the available information, specifically raising a ValueError if the spatial axes have different units.

I think it makes sense to keep this interface which will be mostly useful, but we could start by generalizing, e.g.,

class HasUnits:
    def get_units(self)
        ...

And keep the old behavior:

    def get_xyzt_units(self):
        all_units = self.get_units()
        spatial_units = ...
        temporal_units = ...
        if len(set(spatial_units)) > 1:
            raise ValueError
        if len(set(temporal_units)) > 1:
            raise ValueError
        return spatial_units[0], temporal_units[0]

OTOH, maybe with get_units(), get_xyzt_units() is redundant and can be deprecated.

Thoughts?

@matthew-brett
Copy link
Member

It's attractive to have only one interface to get the units - is the downside just the usual costs of deprecation?

@effigies
Copy link
Member Author

It's more that the upside of having get_xyzt_units() is that you might want a single spatial unit and allow it to raise an exception if it can't be satisfied by the file. Having that check implemented in a single place reduces the odds that a caller gets the check wrong or two different callers might choose different exception types for the same problem.

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

No branches or pull requests

2 participants