Timestep Indexing

[lrennels]

This PR replaced #551, don't merge until after @ckingdon95 's #581

Addresses Issues #549, #568

This PR will handle a few different Issues related to indexing into a Variable or Parameter.

• TimestepValue
  • Base.:+ and Base.:- methods to add/subtract an Int
  • Base.getindex and Base.setindex!
  • ability to index into a TimestepArray with an array of TimestepValues - not for now
  • (?) Colon support i.e. arr[TimstepValue(2000):TimestepValue(2005), 3] - not possible without the context of the TimestepArray
• TimestepIndex
  • Base.:+ and Base.:- methods to add/subtract an Int
  • Base.getindex and Base.setindex!
  • ability to index into a TimestepArray with an array of TimestepIndexs (see need in updated _param_indices function
  • Colon support i.e. arr[TimestepIndex(1):TimestepIndex(5), 3]
• deprecate integer Base.getindex and Base.setindex methods deprecate Int indexing for timesteps #568 ... should make the warning include stack trace info!
• ability to index into a TimestepArray's time dimension with a : (see need in _param_indices special case)
• extra tests and documentation

[corakingdon]

We shouldn't deprecate Int indexing until we also have d.time in the run_timestep function return an iterable of Timestep's that can be used for indexing (instead of returning Ints). Should we also take care of that issue (#538) in this pull request? I started some of the changes for that issue locally, but would be happy to add them here at some point if that makes sense.

[lrennels]

Sure @ckingdon95 do you want to add those changes to this PR? Seems like it might make sense to put them in one?

[corakingdon]

• take a look at better timestep comparison handling run_timestep #337 and handle outstanding issues or close with a comment

Do we need to add comparison methods for these new types now? So that users can compare the AbstractTimestep t given in the run_timestep function with their own constructed types like:

function run_timestep(p, v, d, t)
    if t < TimestepIndex(10) ...
    if t == TimestepValue(2100) ...
end

[lrennels]

I had the same question and wanted to bring that up with you and @rjplevin @davidanthoff. I think adding those methods would probably be doable, but I was getting a little bit confused on use cases because we did employ these primarily for indexing purposes. It seems a little strange to compare t::AbstractTimestep to a non-AbstractTimestep type?

For example, we already have the functions below to help with comparisons ... so what are the use cases here? Do the intersect? Are there new ones?

is_first(t) # returns true or false, true if t is the first timestep to be run
is_last(t) # returns true or false, true if t is the last timestep to be run
gettime(t) # returns the year represented by timestep t
is_time(t, y) # Return true or false, true if the current time (year) for t is y
is_timestep(t, s) # rReturn true or false, true if t timestep is step s.

[corakingdon]

My guess is that we might want to remove is_time and is_timestep, and say that you have to do comparisons with TimestepValue and TimestepIndex objects instead, since the is_time and is_timestep functions only allow the user to test for equality, but not less/greater than. Ultimately, we need @davidanthoff to weigh in here on what we should support!

[lrennels]

Yea I was thinking that we probably don't want both of those options, but I'm not sure yet which I like. As you say, the current way doesn't allow for less/greater than with operators, but you could do that with gettime(t) in some ways, although we don't have get_index(t) as we discussed not wanting ...

[corakingdon]

@lrennels I ended up doing a separate PR for the d.time issue here: #581 just because I thought it would be cleaner to have separate smaller ones, and I have some implementation questions on that one that can be addressed separately. We can probably merge that one before this one though, so that we could still deprecate the Int indexing on this one if you want.

[lrennels]

@ckingdon95 are we deprecating any of the setindex! methods or just getindex? I didn't add methods for setindex! with our new TimestepIndex type but perhaps I need to?

[lrennels]

@rjplevin @ckingdon95 uhoh Houston we may have a problem that needs some creativity ... our crucial mcs function perturb_param! uses integer indexing into TimestepArrays. I could probably use TimestepIndex here in some way (?) but not sure what that will do to performance.

function _perturb_param!(param::ArrayModelParameter{T}, md::ModelDef, 
                         trans::TransformSpec, rvalue::Union{Number, Array{<: Number, N}}) where {T, N}
    op = trans.op
    pvalue = value(param)
    indices = _param_indices(param, md, trans)

    if op == :(=)
        pvalue[indices...] = rvalue

    elseif op == :(*=)
        pvalue[indices...] *= rvalue

    else
        pvalue[indices...] += rvalue
    end
end

[corakingdon]

I think yes, we want to also remove the setindex with Ints functionality, the idea being that users can't use integers for the time dimension for accessing or setting parameters/variables during run_timetsep function.

Regarding the _perturb_params! use-case here, I think there are a couple ways we could handle it:

1. We could have the _param_indices function that's used here return an AbstractTimestep for the time dimension, instead of an Int, or if there's a concern for performance there, then TimestepIndex's might be faster (but I think there is only a performance concern when the TIMES or FIRST fields of the Timestep don't match those of the TimestepArray, which wouldn't be the case here)
2. OR, if we want to still internally use Integer indexing, then maybe we could do: pvalue.data[indices...]. But only TimestepArray's have a data field, and I think that some pvalue's here would also just be normal arrays, so we would have to do a check for that
3. Or, we could creates some _unsafe_Int_getindex function that we allow ourselves to use here, but then it's not exported anymore because it wouldn't be Base.Index

I think I prefer option 1, but we definitely need @rjplevin to weigh in here!

[rjplevin]

Those are all good options to consider. I'd hate to spoil the clean code by introducing a function call rather than splatting the indices, so I prefer #1, returning TimestepIndexes, though we may have to provide some Colon support function for this to work. Not sure....

Let's see if it hurts performance appreciably before we optimize.

[lrennels]

Alright I agree and plan to implement #1. I'm almost there, and have added a setindex! methods for TimestepIndex and made it so that _param_indices returns TimestepIndex types for the :time dimension. The only sticky thing now is that we are often indexing using an array Ints, or in this case now an array of TimestepIndex (see line 58 in test_defmcs). Thus we get the error:

ERROR: MethodError: no method matching getindex(::TimestepArray{FixedTimestep{2015,5,LAST} where LAST,Union{Missing, Float64},2,1}, ::Array{TimestepIndex,1}, ::Int64)

The options I see are to either

1. Write another set of methods for getindex and setindex! that handle arrays (not sure how but it must be doable?)

2. Have _param_indices return something like a vector of individual arrays to iterate over i.e.

[ [TimestepIndex(1), 1], [TimestepIndex(2), 1] ]

which does come with it's own challenges since it requires some flexible splitting deepening how many dimensions we have. Maybe there is a fancy Julia way to do that.

[corakingdon]

@lrennels
I think implementing the getindex for an array of Timesteps makes the most sense. (And I don't think it should be too involved, but not sure...)

And in response to your other question, I also think we need to implement the setindex methods for TimestepValue as well, so that users can use that during run_timestep if needed.

It also looks like some changes need to be made to the test file that's been modified ("test_timesteparrays.jl"). Some new lines have been added to test the new methods, but this has messed up tests that come later that test for certain values that have been reset by the new tests. (ex: lines 52 then 70)

[codecov-io]

Codecov Report

Merging #580 into master will increase coverage by 1.57%.
The diff coverage is 98.7%.

@@            Coverage Diff             @@
##           master     #580      +/-   ##
==========================================
+ Coverage   77.48%   79.06%   +1.57%     
==========================================
  Files          39       39              
  Lines        2572     2718     +146     
==========================================
+ Hits         1993     2149     +156     
+ Misses        579      569      -10

Impacted Files	Coverage Δ
src/Mimi.jl	100% <ø> (ø)	⬆️
src/mcs/montecarlo.jl	81.78% <100%> (+0.14%)	⬆️
src/core/time.jl	96.82% <100%> (+0.39%)	⬆️
src/core/types/time.jl	100% <100%> (ø)	⬆️
src/core/time_arrays.jl	89.53% <98.58%> (+19.86%)	⬆️
src/core/defcomp.jl	89.65% <0%> (-0.08%)	⬇️

---

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 ca424c0...3729377. Read the comment docs.

[lrennels]

Alright team @davidanthoff @rjplevin @ckingdon95 this beast of a PR (although nothing compared to composite components!) is ready to be reviewed. Tests are passing, lots of methods and tests have been added, etc.

I'm putting #337 in a different PR it's too much for this one

[corakingdon]

@lrennels I just noticed that the CodeCov report doesn't look great in terms of how much of the new code isn't being tested. I think we should probably make sure that every method you added gets a test

[lrennels]

fiiiiiiine