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

Refactor some algorithms to improve efficiency and/or cleanup #3078

Closed
wants to merge 57 commits into from

Conversation

DennisHeimbigner
Copy link
Collaborator

Major Changes

  • Convert the loaded_plugin list to use insertion sort plus binary search. This is a straight speed for space tradeoff.
  • Cleanup some dead code.
  • Replace some dicey macros in ncjson.
  • Refactor libdispatch/dutil.c functions to have their own header: ncutil.h.
  • Refactor the byte swapping code.
  • Move to consistently type HDF5 filter ids as int to correspond with H5Z_filter_t. This change excludes ids in the netcdf.h API.
  • Make Zarr format V3 actually use the "bytes" endian handling codec.
  • Fix a weird problem with NC_mktmp.

DennisHeimbigner and others added 30 commits December 23, 2024 15:10
can work with it. I will revise this PR description later.

### Notes:
1. It does not yet run on github actions without errors.
2. It appears to have no memory leaks
3. It has problems with the testing infrastructure that I am working on.
4. CMake does not work for testing v3_nczarr_test because of CMake Policy
   [CMP002](https://cmake.org/cmake/help/latest/policy/CMP0002.html).
5. Ubuntu + Automake appears to work including testing v3_nczarr_test.
6. make distcheck does not work because of difficulties with VPATH.
Add various casts etc to cleanup warnings from
selected modules.

The modules affected are as follows:
* libdispatch
* libncpoco
* libnczarr
* libsrc4
* nczarr_test

Additionally, clean up some Zarr V3 dependencies
in Makefile.am and configure.ac.

Note: This PR is a delta on v3plug.dmh, so cannot be applied
until that previous PR is applied.
H/T Manual Reis
Also turn off some debugging output.
@WardF
Copy link
Member

WardF commented Jan 19, 2025

Hi Dennis, I'll take a look at this but right now I'm focusing on the other giant PR, the V3 one, so that we can get the consolidated metadata work in as well. I've run into an issue on that one as well (more on that on Tuesday), where the libnczarr tests are failing on my local Windows machine. Once we get that sorted I'll take a look at this!

@DennisHeimbigner
Copy link
Collaborator Author

Sigh! I really have to to quit doing these big PRs. It is not fair to you.
What happens is that I am working on something -- adding Zarr V3 for example -- and I
see something that should be fixed. Rather than spinning off a separate PR, I just end up
doing it in the same PR.

@WardF
Copy link
Member

WardF commented Jan 21, 2025

If we could focus on the first v3 PR that would be great, and I see additional failures have crept in here as opposed to the others. Let me see if I can figure out what's going on with that 'out of space' error happening.

WardF and others added 24 commits January 21, 2025 12:00
…error message is congruent with what we observe in the filesystem, at this point.
… in a filesystem which requires it. This may fail on cygwin, but I don't have an easy way to test that, so we shall see what the Github Actions reveal.
re: Unidata#3068 (comment)

At Manuel Reis request, This commit disables the current Zarr-Over-HTTP (ZOH)
support for URLS.
@DennisHeimbigner
Copy link
Collaborator Author

Closing to replace with a set of smaller, more manageable commits.

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

Successfully merging this pull request may close these issues.

2 participants