begin and end: Indexing Interface for TimestepArrays

[lrennels]

This addresses PR #691

@ckingdon95 what was the use case you saw for begin and end? I think beginfor indexing got deprecated in Julia v1.0.0 (https://discourse.julialang.org/t/what-is-the-current-and-future-status-of-begin-keyword-in-indexing/13145) but came back by at least v1.4.0 so I need to sort that out in testing. Regardless it's a little funky so I want to make sure I understand the case/need

I implemented these:

Base.firstindex(arr::TimestepArray) = Mimi.TimestepIndex(1)
Base.lastindex(arr::TimestepArray) = Mimi.TimestepIndex(length(arr))

#TODO

Base.firstindex(arr::TimestepArray, dim::Int)=
Base.lastindex(arr::TimestepArray, dim::Int) =

Which so far allows us to index with syntax like

arr[begin:end] # no idea why someone would use that
arr[begin:TimestepIndex(10)]

Some issues arise however, when we have a multidimensional array and of course only one dimension is time:

• It seems odd to return a TimesteoIndex if the begin or end is not actually in the. time dimension. We could return different types depending on the dimension but that seems like a faux pas
• In these use cases do begin and end generally only concern one dimension at a time ie arr[TimestepIndex(3), 5:end]? Going across dimensions seems odd?

[lrennels]

I removed this line since it's a duplicate of the new method.

[codecov]

Codecov Report

Merging #695 into master will decrease coverage by 0.42%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##           master     #695      +/-   ##
==========================================
- Coverage   80.16%   79.73%   -0.43%     
==========================================
  Files          38       38              
  Lines        2858     2877      +19     
==========================================
+ Hits         2291     2294       +3     
- Misses        567      583      +16     

Flag	Coverage Δ
#unittests	79.73% <20.00%> (-0.43%)	⬇️

Impacted Files	Coverage Δ
src/core/time_arrays.jl	78.12% <20.00%> (-4.41%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbec056...2bea569. Read the comment docs.

[corakingdon]

@lrennels the use case I ran into was in PAGE, where there are components that do

v.varname[end]

in the run_timestep function. This is currently causing the Int indexing deprecation warning, I think because our previous implementation of lastindex was to return an Int. So I think what you've implemented here (to return a TImestepIndex instead) is correct.

I do think we should also get it working for when there are more than one dimension. So the ones you've marked as "todo" would be something like

function Base.lastindex(arr::TimestepArray, dim::Int)
    if dim == ti    # if the requested dimension is the time dimension in the array
        # return the appropriate TimestepIndex
    else
         # return the appropriate integer
    end
end

I think that's what your suggesting, does that make sense?
(also be careful for Arrays with more than one dimension, you'll probably have to use size instead of length to get the right integer)

[lrennels]

Yea, I was thinking of implementing that kind of logic in the leftover methods, but it seemed a little strange to return two different types from the same function but I guess fine since it's such a generic function and specific use case.

I'd need to implement that logic too in the existing methods, because for example

function Base.lastindex(arr::TimestepArray)

if there are multiple dimensions, that the lastindex here is actually also an integer IF time is not the last dimension

[lrennels]

I think for now the use cases best to think about are taking the begin or end in themselves, not using them to splice arrays ie. v.varname[TimestepIndex(3):end] because implementing various colon operators like that starts to get really weird and I want to kick that can down the road.

I can implement the methods above today though and we can merge those.

[lrennels]

@ckingdon95 ok it's ready! All tests do pass on Julia v1.4.0, but I had to comment out the begin tests for now because that keyword was temporarily deprecated for some of the Julia versions we test and thus CI automatically throws errors. I'll reinstate it when we stop testing older Julia versions in the future.

[corakingdon]

This looks great! But it looks like at least the axes method (if not also some of the lastindex/firstindex methods that you added) isn't being hit by any tests. Can you add tests for those lines? (Also what is the axes function?)

I think this is showing what's not covered in red:
https://codecov.io/gh/mimiframework/Mimi.jl/pull/695/diff#D2-115

[lrennels]

The axes function is required by some uses of begin, which I’m not testing right now because they trigger an error in some versions of Julia that don’t support begin (it was deprecated in v1.0.0). I can try to add a version check and check it in v1.4.0 at least.

[lrennels]

https://discourse.julialang.org/t/what-is-the-current-and-future-status-of-begin-keyword-in-indexing/13145

[lrennels]

@ckingdon95 This doesn't seem to be helping, might be because of how the file is parsed, but v1.2.0 and v1.3.0 catch an error in this block. I'll look into it.

[lrennels]

@ckingdon95 I just can't get the parsing to work properly, so per the conversation yesterday I'm commenting out the tests for now and adding a TODO note so we can reenable them as soon as we don't support sub-v1.4.0 versions of Julia. I did double check that the tests are successful on my local machine, which is on v1.4.0.

[davidanthoff]

Tests pass, hurray! @ckingdon95, can you also look over this, and then we merge?