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

dimension scale creation method is in wrong (confusing) location #830

Closed
equaeghe opened this issue Jan 20, 2017 · 4 comments · Fixed by #1212
Closed

dimension scale creation method is in wrong (confusing) location #830

equaeghe opened this issue Jan 20, 2017 · 4 comments · Fixed by #1212

Comments

@equaeghe
Copy link

Currently, dimension scales are created as dataset.dims.create_scale(scale_dataset, 'scale name'), i.e., using the create_scale method of the dims attribute of a Dataset. However, as far as I can tell, dimension scale creation only acts on scale_dataset and has nothing to do with dataset, so it would be more logical to have a create_scale method (which can take a scale name as an argument) on Dataset or perhaps a static method that takes a scale dataset (and a scale name) as an argument.

The current practice is confusing, as it may lead people to believe that the scale is created specifically for dataset, which is not the case, I think. (At least I was confused.)

The change I propose does not need to lead to an API incompatibility: the new method could be added and the old one kept (perhaps in some way deprecated or advised against).

@tacaswell
Copy link
Member

This looks like it could be converted into a static method on the dims proxy, however it is using an instance method to encode the name which may pull information from the underlying object (but it looks like this code path does not).

Having the create_scale method on the dims proxy makes a fair amount of sense to me, it provides a name space so we don't have too many loose functions floating around.

At a minimum it should be better documented.

@takluyver
Copy link
Member

I agree with @equaeghe that it's somewhat confusing to have this method associated with an uninvolved dataset.

Instead of this:

f['data'].dims.create_scale(f['x2'], 'x2 name')

It would conceptually align with HDF5 better to do something like this:

f['x2'].make_scale('x2 name')

Or it could be an option when you create a dataset - this makes the intent clearer to my mind, though it's somewhat less flexible.

f.create_dataset('x2', data=np.arange(10), as_scale='x2 name')

@rmvanhees
Copy link

rmvanhees commented Oct 24, 2018 via email

@tacaswell tacaswell modified the milestones: 2.9, 2.10 Nov 8, 2018
takluyver added a commit to takluyver/h5py that referenced this issue Apr 26, 2019
Replaces other_ds.dims.create_scale(ds), which is deprecated here.

Closes h5pygh-830
@takluyver
Copy link
Member

I've opened PR #1212 to address this.

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

Successfully merging a pull request may close this issue.

5 participants