WIP Allow any placement of time dimension

[corakingdon]

Description of changes:

• TimestepArrays now have an additional type parameter, ti which is an integer value equal to the time index position of that array. (I'm not particularly attached to the name ti, we can pick something better if we want)
• additional getindex and setindex functions for TimestepMatrix with ti equal to 2 were added
• getindex and setindex functions for TimestepArrays have all been changed, similar to the following example:

function Base.getindex(arr::TimestepArray{FixedTimestep{FIRST, STEP}, T, N, ti}, idxs::Union{FixedTimestep{FIRST, STEP, LAST}, AnyIndex}...) where {T, N, ti, FIRST, STEP, LAST}
	idxs1, ts, idxs2 = idxs[1:ti - 1], idxs[ti], idxs[ti + 1:end]
	return arr.data[idxs1..., ts.t, idxs2...]
end

• there's a helper function called get_time_index_position that gets used during build to get the appropriate ti value based on the parameter or variables dimension order.

Todo:

• I've written tests locally that test most of the functionality, but I want to place them in appropriate test files rather than adding an additional test file.
• I tried to get explorer working (right now it displays values as a bar graph if time is the second dimension) but I haven't pushed those changes yet because I haven't got it to work
• we might want to implement some better error messaging if users mess up their dimension ordering

[rjplevin]

idxs1, ts, idxs2 = idxs[1:ti - 1], idxs[ti], idxs[ti + 1:end]

Personal taste, but I find this "3 assignments in one" formulation less readable than 3 simple assignments on separate lines.

Also, should the new parameter be TI in uppercase for consistency?

[rjplevin]

For cases like these:

function next_timestep(ts::FixedTimestep{FIRST, STEP, LAST}) where {FIRST, STEP, LAST}
	if finished(ts)
	    error("Cannot get next timestep, this is last timestep.")
	end
	return FixedTimestep{FIRST, STEP, LAST}(ts.t + 1)
end

function next_timestep(ts::VariableTimestep{TIMES}) where {TIMES}
	if finished(ts)
	    error("Cannot get next timestep, this is last timestep.")
	end
	return VariableTimestep{TIMES}(ts.t + 1)		
end

Can't we have a single function:

function next_timestep(ts::T) where {T <: AbstractTimestep}
	if finished(ts)
	    error("Cannot get next timestep, this is last timestep.")
	end
	return T(ts.t + 1)		
end

And similarly for pairs of functions that don't do anything with the more detailed type parameters, i.e., they just call already-type-differentiated functions like finished(). Whenever the body of the two related functions is identical, we should be able to consolidate.

[corakingdon]

• generally I would agree that the triple assignment reads worse, but in this case it appears so many times in the "time.jl" file that I personally prefer the brevity. (an alternative would be to define a helper function like split_index(idxs, ti) that returns all three that we could use everywhere. thoughts?)
• happy to go with capital TI
• you're definitely right about the pairs of functions, although that's not completely related to this PR. but I could make those changes here now if we want (since it applies to several of the functions modified here)

[corakingdon]

also just a note on the innards of "time.jl": some of the functions for VariableTimesteps that looked like this:

function Base.getindex(mat::TimestepMatrix{VariableTimestep{D_FIRST}, T, 1}, ts::VariableTimestep{T_FIRST}, idx::AnyIndex) where {T, D_FIRST, T_FIRST}
	t = ts.t + findfirst(isequal(T_FIRST[1]), D_FIRST) - 1
	data = mat.data[t, idx]
	_missing_data_check(data)
end

I changed to this:

function Base.getindex(mat::TimestepMatrix{VariableTimestep{D_TIMES}, T, 1}, ts::VariableTimestep{T_TIMES}, idx::AnyIndex) where {T, D_TIMES, T_TIMES}
	t = ts.t + findfirst(isequal(T_TIMES[1]), D_TIMES) - 1
	data = mat.data[t, idx]
	_missing_data_check(data)
end

because I believe the parameter names D_FIRST and T_FIRST were misleading and that D_TIMES and T_TIMES are more appropriate. This doesn't change any functionality, I just noticed this while I was going through all of these functions.

[rjplevin]

Defining a short function split_index(idxs, ti) sounds like a good approach. (Short funcs get inlined anyway, so there's no penalty.) It would be both readable and concise.

I'm agnostic about whether we consolidate the pairs of functions in this PR or in a separate one.

[rjplevin]

For some reason, I'm not seeing an "Approve" button, but I think you can merge this if tests are all passing. Nice job!

[codecov-io]

Codecov Report

Merging #434 into master will increase coverage by 0.23%.
The diff coverage is 58.42%.

@@            Coverage Diff             @@
##           master     #434      +/-   ##
==========================================
+ Coverage   80.73%   80.97%   +0.23%     
==========================================
  Files          26       26              
  Lines        1936     1976      +40     
==========================================
+ Hits         1563     1600      +37     
- Misses        373      376       +3

Impacted Files	Coverage Δ
src/core/types.jl	94% <ø> (ø)	⬆️
src/core/defcomp.jl	89.5% <ø> (+0.06%)	⬆️
src/core/defs.jl	91.98% <100%> (+0.02%)	⬆️
src/core/build.jl	98.87% <100%> (+0.01%)	⬆️
src/core/connections.jl	90% <100%> (+7.76%)	⬆️
src/explorer/buildspecs.jl	52.57% <16.66%> (-0.69%)	⬇️
src/core/time.jl	75.6% <50%> (-7.36%)	⬇️
src/core/model.jl	48.27% <0%> (+2.29%)	⬆️
... and 1 more

---

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 32aaa3b...e5172a3. Read the comment docs.

[corakingdon]

Okay I'll make those few changes but I'm not going to merge it until I add more tests! and get explorer working (maybe I can elicit @lrennels for that)

[lrennels]

@ckingdon95 happy to help if things are confusing in explorer. It assumes that if there is a time dimension, and thus it needs to pick a line or multiline plot, that time is the first dimension. The easiest way to fix it is probably to expand that check in _spec_for_item on line 33 of buildspecs.jl and then rearrange the dataframe that is passed to createspec_multiline or createspec_line. Happy to continue to discuss!

[corakingdon]

@lrennels I merged master back into this branch so the tests just re-ran. It looks like some of the dependency tests failed on Travis, but not for all of the deployments. You're more familiar with these tests than I am, would you mind taking a look and seeing if you think this is caused by something on this branch, or if this is just an unrelated Travis problem? Thanks!!

[lrennels]

@ckingdon95 we're getting this exact same error in the current Mimi build, so I don't think there's an issue with this branch! I will look into what's going on with the Mimi build but it seems you could merge this and we'd be in the same place we are already :)