metrumresearchgroup / bbr

R interface for model and project management
https://metrumresearchgroup.github.io/bbr/
Other
22 stars 2 forks source link

get_param: fix data length for constructed matrix #626

Closed kyleam closed 7 months ago

kyleam commented 7 months ago

When creating the matrix from a vector and labels, format_matrix() calculates the data size as 2 N, where N is the maximum label index (e.g., 2 3 for a 3x3 matrix). The correct size is N * N.

Fortunately that error doesn't functionally matter because matrix's ncol and nrow arguments, not the input data length, control the final shape.

R 4.2 or later flags the issue in cases where 2 N isn't a multiple of N N. Several spots in test-get-param.R now trigger a warning: "data length differs from size of matrix".

Fix the issue by dropping the rep() call entirely and passing a scalar. matrix() will repeat that as many times as needed.

kyleam commented 7 months ago

As an aside, you shouldnt be able to have a 2 x 3 matrix for an OMEGA matrix from my understanding though (not sure if that was just an example).

I think you misunderstood the first paragraph of the commit message. It's describing how the current state is incorrect. The whole point is that 2 * N is wrong, and the size calculation should be N * N (i.e. the size of a square matrix).


Given that's the core point, do you want to take another look in light of that and make sure you still approve?

Also, let me know if you have a suggestion for rephrasing the commit message in a way that you would find clearer.

barrettk commented 7 months ago

@kyleam No I see what youre saying by that - I agree that it's wrong. I honestly recall thinking about this when I implemented it (and knew it should be N x N in terms of dimension). I wish I could remember why I changed it to 2 x N, but now I am not able to make sense of why I did it that way. Would bet I saw some weird behavior in the console or something, but dont think it really matters at this point. Regardless, a single static value makes the most sense since the row and column specification ultimately puts it in the correct format/dimension.

But no I think your commit message is fine and still approve, I just wanted to make sure a 2 x 3 matrix wasnt a possible output from get_omega()/get_param().

kyleam commented 7 months ago

@barrettk Got it. Thanks for the review.

(Obviously this is just a focused fix, but I wouldn't be opposed to a rewrite of format_matrix that makes it bit clearer that this should always be a square matrix. For example, get max index for rows and columns and assert they're the same.)