Fix list-to-array conversion for integer arrays - #1225
Merged
Conversation
process_data() coerced variables declared as int with mode<-, which errors on lists and data frames. The list-to-array conversion happens later, in write_stan_json(), so the documented list convenience worked only for real variables. Extract the container conversions into convert_to_array(), shared by both functions, and apply them in process_data() before the length-1 and int-coercion checks. Document factor conversion, which was undocumented, and correct the claim that lists cannot be used for arrays with more than one dimension. A list contributes exactly one leading dimension, so array[K,L] vector[J] can be supplied as a list of K LxJ matrices; the real limitation is that nested lists are not supported. closes #817
data.matrix() silently coerced character columns to factor codes and date/time columns to numeric, so a value the data list would reject as a variable was accepted as a column. Apply the same type check per column.
list_to_array() rejected logical elements even though logical variables and logical data frame columns are both accepted. Allow them, and apply the logical-to-integer conversion after the container conversions so a list of logicals is not written as JSON true/false. Factors are written as their level indices, which are only meaningful for variables declared as int. Error when one is supplied for any other type.
array() drops the factor class, so the length-1 reshaping left a character array behind and factors failed for array[1] int. The factor check also ran after data.matrix(), so a factor column supplied for a real variable was silently converted to level codes. Move factor handling ahead of both conversions: convert to integer for int variables, and error for any other type, including data frames with factor columns. Describe level ordering in terms of levels() rather than "alphabetical", which is wrong for numeric input, and correct the list element error message now that logical elements are accepted.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1225 +/- ##
==========================================
+ Coverage 91.79% 91.90% +0.11%
==========================================
Files 15 15
Lines 6200 6260 +60
==========================================
+ Hits 5691 5753 +62
+ Misses 509 507 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Submission Checklist
Summary
This PR and the description below was co-written by me and Claude (most of the code was written by me directly and checked by claude, most of the text was written by Claude directly and checked by me)
Fixes #817.
process_data() makes sure variables declared as int are of integer type by calling mode<- on them, which errors on lists and data frames. The list-to-array conversion that would make those inputs atomic happens later, in write_stan_json(), so it never had a chance to run. The result was that the documented list convenience worked only for real variables: a list of matrices for array[K,I,J] real was fine, while the same list for array[K,I,J] int failed. Data frames failed for int variables for the same reason.
This moves the container conversions (table, logical, data.frame, list) out of write_stan_json() into a convert_to_array() helper shared by both functions, and calls it in process_data() before the length-1 and int-coercion checks. Two smaller things fall out of the reordering: the length-1 check now measures the converted array rather than counting list elements, and a list containing NA reports "has NA values" instead of failing inside a coercion.
While in there we also tightened three cases where the same value was accepted in one position and rejected or silently coerced in another:
Documentation: factor handling was not documented anywhere, so write_stan_json() now describes it, including that the indices depend on level order rather than on the values (factor(c(10, 9, 8)) is written as [3, 2, 1]). It also corrects the claim that a list cannot be used for arrays with more than one dimension. A list contributes exactly one leading dimension, so array[K,L] vector[J] v can be supplied as a list of K LxJ matrices; the real limitation is that nested lists are not supported.
One limitation is inherent rather than deferred. write_stan_json() called directly cannot do the factor check, because it has no variable declarations to compare against, so it still converts factors silently. Only the fitting methods of a model compiled from a Stan file have the information needed. This is noted in the write_stan_json() documentation.
Copyright and Licensing
Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
Jonah Gabry
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses: