WIP: Allow Indexing into a Variable/Parameter with a Year

[lrennels]

UPDATE: CLOSING THIS PR TO START FRESH

#549, #568, #337

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

• create the two new types
• add Base.:+ and Base.:- methods for the two types
• add getindex methods to TimestepArray, TimestepVector, and TimestepMatrix
• deprecate int indexing (deprecate Int indexing for timesteps #568)
• take a look at better timestep comparison handling run_timestep #337
• time_index
• tests and documentation

[lrennels]

@davidanthoff @rjplevin here's a first pass at these, I will go back over it and see if I can make things clearer or faster, as well as add tests and docs, but for now wanted to get the general ideas down and see what you guys think?

[codecov-io]

Codecov Report

Merging #551 into master will decrease coverage by 1.05%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #551      +/-   ##
==========================================
- Coverage   83.81%   82.75%   -1.06%     
==========================================
  Files          27       27              
  Lines        2125     2180      +55     
==========================================
+ Hits         1781     1804      +23     
- Misses        344      376      +32

Impacted Files	Coverage Δ
src/core/time.jl	63.45% <0%> (-12.77%)	⬇️
src/core/types.jl	93% <0%> (-0.94%)	⬇️
src/utils/getdataframe.jl	91.76% <0%> (+0.09%)	⬆️
src/core/model.jl	48.86% <0%> (+0.58%)	⬆️
src/core/build.jl	100% <0%> (+1.12%)	⬆️
src/mcs/montecarlo.jl	81.92% <0%> (+1.75%)	⬆️

---

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 e0a6563...b10b69d. Read the comment docs.

[davidanthoff]

Just to record what we decided in our meeting today: we'll have two new types, one called TimestepValue and one called TimestepIndex. And we'll wait with this PR until the composite components have been merged into master.

[corakingdon]

I was still feeling unsure about these type names, TimestepValue and TimestepIndex since the thing that gets constructed is actually a Timestep, not a value or an index. What if instead we offer two keyword options: Timestep(value = 1952) or Timestep(index = 2)? Or are the types we are creating for this actually two different things? Not super important, but was wondering what you guys think of that instead. (I'm also not even sure if you can have keyword arguments like that in a constructor) @davidanthoff @lrennels @rjplevin

[rjplevin]

@ckingdon95, I see it slightly differently. The problem is that we have two conceptually different arguments, both integers, that we want to distinguish. If we define:

struct TimestepIndex
  value::Int
end

struct TimestepValue
  value::Int
end

we've effectively created two new type-system-distinguishable integers. So I don't see TimestepIndex(2) as producing a Timestep, but producing an integer value of 2 "tagged" to be interpreted as an index rather than as a year.

P.S. Yes, you can have keyword args on constructors!

[corakingdon]

Ah I see. If we are creating two different types then that makes sense. I thought that both of these would create the Timestep type that Lisa added and that both would hold the value and offset_in_timesteps fields. If we're creating the two different types you've shown here though, is it obvious how we resolve the arithmetic operations we discussed?

[davidanthoff]

We should probably make it

struct TimestepValue{T}
  value::T
end

so that one isn't limited to Int for the value domain here, but could instead also use a Date or something, if one wanted to.

[rjplevin]

Agreed about type parameterization.

As we've discussed, a time value, e.g., 2005, cannot be used without reference to the actual time dimension definition. So I don't see how TimestepValue(2005) would be converted to a timestep. It could, however, be interpreted w.r.t. a time dimension, as could TimestepIndex(2).

If we define these as you discussed on the call, with an offset field, e.g.,

struct TimestepValue{T1, T2}
  value::T1
  offset::T2
end

struct TimestepIndex{T1, T2}   # actually, mustn't these both always be Ints?
  value::T1
  offset::T2
end

then the add/subtract functions just operate on the offset. The whole thing is ultimately interpreted w.r.t. a given time dimension.

[davidanthoff]

Yes, exactly, TimestepValue can only be resolved to an index in combination with a time array, which has the necessary offset info.

And yes, I think the fields in the TimestepIndex must be Int. Same for the offset in TimestepValue.

[corakingdon]

Does TimestepIndex even need the offset field then? Can't we just do arithmetic by incrementing it's value?

[rjplevin]

That sounds right. I suppose we could also say that if it's an Int, it's an index, and treat the TimestepValue(x) differently. I do prefer the symmetry and lack of ambiguity of the two types, though.

[corakingdon]

Maybe it should be:

struct TimestepValue{T}
  value::T
  offset::Int
end

struct TimestepIndex
    index::Int
end

Even less symmetrical, but maybe more clear?

[davidanthoff]

Yes, I think that is how it should look.

[lrennels]

This looks good to me, although @davidanthoff when you say that we can type parameterize TimestepValue with something like Date, that would only work if we add functionality so that the TimestepArray holds Date types instead of Int types right? Because right now this function is key to finding the index, but obviously uses a lookup in the array:

# Helper functions for Timestep type
function _get_time_value_position_(times::Array, ts::Timestep)
	t = findfirst(isequal.(ts.value, times))
	if t === nothing
		error("cannot use Timestep with value $(ts.value), value is not in the TimestepArray")
	end
	
	t_offset = t + ts.offset_in_timesteps
	if t_offset > length(times) 
		error("cannot get offset of $(ts.offset_in_timesteps) from $(ts.value), offset is after the end of the TimestepArray")
	end
	return t_offset
end

[corakingdon]

I think it has to do with what type is used when the user calls set_dim!(m, :time, ...). So far, all our models have Vector{Int} as the time dimension, but it could be Dates. I think there are places in the code that would have to be updated to support this, but I think this _get_time_value_position_ function should still work, since I'm assuming isequal work for Dates, and the times parameter would be a vector of Dates if that was the specified type.

[lrennels]

Agreed!

[corakingdon]

@lrennels should we also add a getindex(t) function (similar to the gettime function) that returns the index value? This wouldn't work on TimestepValue objects, but should work on TimestepIndex, FixedTimestep and VariableTimestep. I think the use of this would just be in the run_timestep function if someone wanted to know if they were on a certain timestep other than the first or last, e.g. the second timestep, like getindex(t) == 2. I vaguely remember this being discussed somewhere else, but I can't remember where, so sorry if this was already resolved!

[rjplevin]

We should call it something else since Base.getindex is the array access method. Maybe time_index?

[davidanthoff]

I think I would actually prefer that such a test is written as t == TimestepIndex(2), rather than us providing a way to extract the index value. On some level, we should really discourage the use of these index values as much as possible...