I am wondering if the "length check" can't be just part of the helper? We use the same error in any case?
Could even usePySequence_Fast in the helper, but that doesn't really matter.

That would mean that the helper should probably returnnum_usecols and fill inusecols (passed by pointer).

Can we move this toconversion_utils.c? There is similar functionality there already, and now that it is split out, maybe it is time to just put it there.

Would it be ugly to move all usecols handling here? Its fine, but conditional ownership tens to be an anti-pattern.

Another way to spell would be to haveusecol_from_func = ... andusecol = usecol_from_func, so the owner is unconditionallyusecol_from_func and not "usecol" itself.

Would it be ugly to move all usecols handling here?

No, I think that would be fine. In fact, with a bit more of the argument processing code moved intoread_rows(), I think the function_readtext_from_stream() can be eliminated, and_load_from_filelike() can callread_rows() directly. I'll give that shot, and push to the PR if it looks reasonable.

We are missing a test (and I think also the logic) to reject a change in the number of columns when a callable is given. This is a bit of a tricky case!

The problem is that ifn changes, the return value of theusecol function might also change! While normally, when usecols are given we do not have to worry about that at all.

The example would be something like:

np.loadtxt(["1 2", "1 2 3"], usecols=lambda n: range(n)[-2:])

As you can see in the code, I've been working under the assumption that once the first row has been read, the value ofn is determined once and for all. If given, the user-defined callable is called justonce, after the first row is read. If a structured dtype is given,n is the number of fields in the dtype; otherwisen is the number of columns found in the first row of the file. That's because I didn't think we had such strong support for ragged text files. I wish we didn't, and I don't recall the history of how we got here, but if that's where we are now, so be it. (There is no formal specification for the format of the text files that we support, so the scope ofloadtxt tends to creep in fits and starts.)

It seems that a simple way to handle this is to cache the results of the callableusecols in a dictionary, and look up the result after each line is parsed. We only actually call the function when a new number of columns is encountered. There will have to be a check added to ensure that the length of the sequence returned by subsequent calls is the same as that of the first call. What do you think?

I don't mind raising an error. We already do ifusecols isnot given. We do have oddly strong support for changing number of columns, simply becauseloadtxt used to use list indexingfor col in usecol: line.split(delimiter)[col].

Yes, you could cacheusecols(n) with changingn (in the unlikely event that it chagned). Or... you just raise an error :).

I would be OK even deprecating that whole thing. Just dropping support fornegative usecols together withragged columns, might simplify things also.

Probably makes sense to reorganize the conditions so that the outermost can beif (NPY_UNLIKELY(current_num_fields != prev_num_fields)) {.
There is a lot of code here now in a relatively hot path, so lets help the compiler shovel it to the side somewhere.

(Could move most of this into a helper as well probably).

This assert is failing (if you are looking for that).num_usecols is not necessarily defined here, is it?

Ah, the result of an incomplete clean up. I just removed the assert().