8000 Default to phony_dims="access" in h5netcdf-backend by kmuehlbauer · Pull Request #10058 · pydata/xarray · GitHub
[go: up one dir, main page]
Skip to content

Default to phony_dims="access" in h5netcdf-backend #10058

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Feb 24, 2025
Merged

Default to phony_dims="access" in h5netcdf-backend #10058

merged 13 commits into from
Feb 24, 2025

Conversation

kmuehlbauer
Copy link
Contributor
@kmuehlbauer kmuehlbauer commented Feb 18, 2025 •
edited
Loading
  • Warn user about behaviour change.

@kmuehlbauer kmuehlbauer changed the title Default to phony_dims="access" in open_datatree for h5ntecdf-backend Default to phony_dims="access" in open_datatree for h5netcdf-backend Feb 18, 2025
@kmuehlbauer kmuehlbauer mentioned this pull request Feb 18, 2025
Closed
@kmuehlbauer
Copy link
Contributor Author

I'm not sure, if conditionally warning about the change is appropriate, as this will annoy users which are not affected by this (conforming netcdf4 files, no unassigned dimension scales).

OTOH, if we switch to either of sort/access users will not get notified about the file missing dimension scales. I'm testing now, if there are any phony_dims found in the file and only warn in these cases. This should make users aware about the change.

For future versions we might remove this check, but users won't get notified having "phony_dims" in their data. But this might not be a problem, as netcdf-c/netCDF4-python does not warn either.

@flamingbear @eni-awowale What's the best approach from your side?

@kmuehlbauer kmuehlbauer marked this pull request as ready for review February 18, 2025 09:35
@eni-awowale
Copy link
Collaborator

Hi @kmuehlbauer,

if we switch to either of sort/access users will not get notified about the file missing dimension scales. I'm testing now, if there are any phony_dims found in the file and only warn in these cases. This should make users aware about the change.

So use phony_dims='access' by default and only warn users if there are phony_dims in the file? If I am understanding correctly, I think this approach should meet our needs at GES DISC.

kmuehlbauer
kmuehlbauer commented Feb 18, 2025
View reviewed changes
@kmuehlbauer
Copy link
Contributor Author

@eni-awowale Yes, that's the idea.

Should this be done for open_dataset, too? To be more or less congruent here? I could highlight this more in the in the docstrings, then.

@kmuehlbauer kmuehlbauer changed the title Default to phony_dims="access" in open_datatree for h5netcdf-backend Default to phony_dims="access" in for h5netcdf-backend Feb 19, 2025
@kmuehlbauer kmuehlbauer changed the title Default to phony_dims="access" in for h5netcdf-backend Default to phony_dims="access" in h5netcdf-backend Feb 19, 2025
flamingbear
flamingbear approved these changes Feb 19, 2025
View reviewed changes
Copy link
Member
@flamingbear flamingbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for jumping on this. I think it looks good.

@flamingbear
Copy link
Member

I had to go understand phony_dims, but I agree that having a default value for the user will ease some of the friction. I didn't see much difference is performance on large ICESat-2 data and it sounds like "access" is a good option.

@kmuehlbauer
Copy link
Contributor Author

Thanks @eni-awowale and @flamingbear for your input. I'll leave this open for another couple of days before merging.

@kmuehlbauer kmuehlbauer added the plan to merge Final call for comments label Feb 20, 2025
@kmuehlbauer kmuehlbauer merged commit cf0d0ea into pydata:main Feb 24, 2025
37 checks passed
@kmuehlbauer kmuehlbauer deleted the phony_dims_access branch February 24, 2025 07:27
@kmuehlbauer
Copy link
Contributor Author

Thanks @flamingbear and @eni-awowale for the review and approval!

@eni-awowale
Copy link
Collaborator

Thanks @kmuehlbauer for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flamingbear flamingbear flamingbear approved these changes

Assignees
No one assigned
Labels
plan to merge Final call for comments
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

phony_dims must be specified when opening HDF5 file without dimension scales
3 participants
@kmuehlbauer @eni-awowale @flamingbear
0