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

allow empty chunks #220

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

allow empty chunks #220

wants to merge 1 commit into from

Conversation

rafaqz
Copy link
Collaborator

@rafaqz rafaqz commented Jan 30, 2025

This needs tests, but wonddering @meggart is there a reason this isn't allowed? It seems to come up sometimes that arrays end up empty and filtering those cases out is more annoying than just handling the empty case

@meggart
Copy link
Collaborator

meggart commented Jan 31, 2025

I don't think empty arrays are a problem. It is just a zero chunk size that IMO does not make a lot of sense. I have not been following issues in the last week, is there an example where this came up?

@rafaqz
Copy link
Collaborator Author

rafaqz commented Jan 31, 2025

I will have to investigate, but somehow I'm hitting this error with regular code, but it's deeply nested so not easy to demo.

Maybe it needs to be caught further out, I can try that approach.

@meggart
Copy link
Collaborator

meggart commented Feb 3, 2025

Yes, this would be good to investigate. Usually this happens when there are some fallback definitions for eachchunk for custom array types that go like RegularChunks(s,0,s) where s is the length along the dimension, which should rather be RegularChunks(max(s,1),0,s). I wonder if we could modify the inner constructor of RegularChunks slightly to something like this:

struct RegularChunks <: ChunkType
  cs::Int
  offset::Int
  s::Int
  function RegularChunks(cs::Integer, offset::Integer, s::Integer)
      if cs < 0 
        throw(ArgumentError("Chunk sizes must be strictly positive"))
      elseif cs == 0
        if s == 0
          #This is a special case we allow and where we set the chunk size to 1
          cs = 1
        else
          throw(ArgumentError("Chunk sizes must be strictly positive"))
        end
      end
      -1 < offset < cs || throw(ArgumentError("Offsets must be positive and smaller than the chunk size"))
      s >= 0 || throw(ArgumentError("Negative dimension lengths are not allowed"))
      new(Int(cs), Int(offset), Int(s))
  end
end

So we repair the special case when both chunk size and dimension length are 0. No idea what we should do here, but would definitely be good to find out where the zero-chunksize is coming from. I can investigate as well, even if it is nested, would just be good to have a pointer. I think I have seen this error when creating empty slice views into Rasters, but can not remember details.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Feb 12, 2025

Probably resolved by #232, but we should check

@meggart
Copy link
Collaborator

meggart commented Feb 12, 2025

I would suggest to leave this PR as stale for now and see if problems keep occurring after #232 . If problems are still reported, lets discuss this again.

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