julia icon indicating copy to clipboard operation
julia copied to clipboard

Implement and export `isfull`

Open KronosTheLate opened this issue 1 year ago • 2 comments

This PR implements isfull(c::Channel).

It calls n_avail(c) ≥ c.sz_max in all cases. The original implementation was inspired by this comment, and therefore had a special case for unbuffered channels, which fell back to isready. I opted against this behaviour, because it fails to respect that an unbuffered channel is always full, in two important senses: 1) The number of elements available is greater than or equal the capacity 2) A call to put! will block

With the current implementation, the behaviour is simply understood and summarized in all cases by the start of the docstring:

Determines whether a Channel is full, in the sense that calling put!(c, some_value) will block.

Shoutout to @SamuraiAku for their work in https://github.com/JuliaLang/julia/pull/40720, which helped me a lot on thinking this through, and remembering to change all relevant files. In particular, the detail around how c.cond_take.waitq may result in immediate unblocking, which is a really important caveat on a function that may be used to check if put!ing will block. However, for buffered channels, isfull is extremely close to putwillblock from #40720 (just a little better, with >= instead of ==), and for unbuffered channels it does not make much sense to see if put!ing will block.

This PR is created based on this "call to action".

Checklist:

  • [x] Entry in news
  • [x] Docstring with example
  • [x] Export function
  • [x] Mention in manual
  • [x] Entry in docs-reference

KronosTheLate avatar Feb 02 '24 13:02 KronosTheLate

If this goes through, let's not forget https://github.com/JuliaLang/julia/pull/40921 (Equivalent functionality for RemoteChannel), which is waiting for https://github.com/JuliaLang/julia/pull/40720, even if merging this closes https://github.com/JuliaLang/julia/pull/40720.

KronosTheLate avatar Feb 02 '24 13:02 KronosTheLate

@KronosTheLate Thanks for the shoutout!

SamuraiAku avatar Feb 03 '24 06:02 SamuraiAku

Bump

KronosTheLate avatar Feb 28 '24 21:02 KronosTheLate

I would not find the changes made here in the 1.11 alpha release notes. Is further action required for this to make it into there?

KronosTheLate avatar Mar 05 '24 08:03 KronosTheLate

(FWIW, I think this should have been / should be more descriptive. isfull made me think it was the opposite of issparse.)

KristofferC avatar Mar 05 '24 13:03 KristofferC

(FWIW, I think this should have been / should be more descriptive. isfull made me think it was the opposite of issparse.)

Fair point. I expect that many people who regularly work with matrices, which are a lot of people, would feel the same. Importantly, people who use Julia are more likely to also be people who regularly work with matrices. So I expect this will be an issue for others as well.

I do however have to object. The opposite of full is empty, not sparse. Full/empty is a more natural dichotomy than full/sparse. The current name has the most natural meaning, without being so specific as to become hard to discover. Also, would the opposite of sparse not be dense? I may be wrong of course, but I feel quite certain that the name is fine.

KronosTheLate avatar Mar 05 '24 13:03 KronosTheLate

Yeah, thinking about it some more, I guess isfull has a generic concept like isempty so this is probably fine.

KristofferC avatar Mar 05 '24 14:03 KristofferC