nilsbeck/add time dependent matrices #66
Conversation
|
|
||
| // TryAssertFloat64Matrix tries to assert that the given matrix is a | ||
| // [][]float64. Returns true if successful and the matrix otherwise false. | ||
| func TryAssertFloat64Matrix(matrix []any) ([][]float64, bool) { |
There was a problem hiding this comment.
Should this also check whether the matrix is square?
There was a problem hiding this comment.
this is not done here, but in the validateMatrix function here to re-use the code we were using already:
Line 127 in 27a581b
nextroute/schema/input.py
Outdated
| scaling_factor: Optional[float] = None | ||
| """Scaling factor for the time frame.""" | ||
|
|
||
| class DurationMatrices(BaseModel): |
There was a problem hiding this comment.
Can we rename this something more specific? DurationMatrices suggests to me that it is simply a slice of duration matrices, but it is much more complex than that.
If we are planning to extend the features further in future, I would probably even go for something like CustomizedMatrix. There is probably a better name for it. If it is only features controlled by time frames, maybe we just call it TimeBasedMatrix or TimeDependentMatrix to be in line with other language we use.
nextroute/schema/input.py
Outdated
| @@ -29,6 +30,24 @@ class DurationGroup(BaseModel): | |||
| group: List[str] | |||
| """Stop IDs contained in the group.""" | |||
|
|
|||
| class TimeFrame(BaseModel): | |||
There was a problem hiding this comment.
Maybe we can rename this struct to MatrixTimeFrame or something a little more specific. Just to make sure it won't get mixed up with something else, as this feels like a very custom feature and not very generic.
schema/input.go
Outdated
| // DurationMatrices represents time-dependent duration matrices. | ||
| type DurationMatrices struct { |
There was a problem hiding this comment.
Same naming comments as with the Python code. 😊
schema/input.go
Outdated
| // TimeFrame represents a time-dependent duration matrix or scaling factor. | ||
| type TimeFrame struct { |
There was a problem hiding this comment.
And here. I should have commented on the Go code first. 😅
There was a problem hiding this comment.
Can we add a very brief markdown file on what we expect for these test cases? Similar to multi_window.md and such. Really not required, but I have trouble following such complex golden file tests and am concerned that we loose the tests intention accidentally over time.
Can totally be shorted as well. No need to copy the content of above example, just having some comment in a equally named .md feels like it may help these test cases. 😊
| jsonData, err := json.Marshal(matrix) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| err = json.Unmarshal(jsonData, &durationMatrices) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
I know we do this to simplify conversion, and I am not opposing it. Just wondering: do we know whether this becomes a performance problem at a certain size that we think is still realistic to have? 🤔
(saw this being used before; just a thought - not blocking at all)
Description
Up until now it was only possible to add one single duration_matrix in the input which was being used for all vehicles (when given).
To be more flexible and cover more use cases this PR allows to pass more matrices.
schema.DurationMatricescan be used to model time dependent matrices for all vehiclesIn this scenario the vehicle ids have to be blank, the time frames can be set to create time dependencies for the default matrix. Within a TimeFrame you can either pass a new matrix or scale the default matrix with a factor. The specific matrix will be applied for the time frame, specified by a start and end date.
schema.DurationMatricesto create multiple time dependent matrices for different vehicles. This is done be specifying the vehicle ids in the input so that every vehicle gets assigned the corresponding time dependent matrices.