-
Notifications
You must be signed in to change notification settings - Fork 23
Add R backend-controlled format options to data explorer #987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
That's great — nice work! 👍I’ll try out your way, but still hope the official team jumps in to fix it. |
Cool! You will probably need to build both Ark and Positron for it to take effect. I'll have my fingers crossed that you don't run into compilation errors! |
dfalbel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @kv9898 !
We should add a test in https://github.com/posit-dev/ark/blob/f0a0e689892e8a45c2b3b2ef9978d8c805e8c804/crates/ark/tests/data_explorer.rs checking if the state is changed after changing that option.
You can use eg: https://github.com/kv9898/ark/blob/9c14a03e3e517e9b3534a2a9a6fcc428a7b707ab/crates/ark/tests/data_explorer.rs#L826
|
@dfalbel It is actually impossible to 100% replicate the R behaviour for (at least) 2 reasons:
I have rewritten the logic to match with the scalar printing behaviour in R using the following examples # Set the scipen option to a specific value
options("scipen" = 0, "digits" = 7) # default to 0 and 7
test <- data.frame(num = c(0.000145, 0.0000145, 10000, 100000, 12345.123456))
print(0.000145) # fixed notation
print(0.0000145) # scientific notation in R, 6 d.p. in Positron
print(10000) # fixed notation
print(100000) # scientific notation
print(12345.123456) # 2 d.p.
options("scipen" = 1, "digits" = 7) # scipen to 1
print(0.000145) # all fixed
print(0.0000145)
print(10000)
print(100000)
print(12345.123456) # 2.d.p.
options("scipen" = 0, "digits" = 8) # digits to 8
print(0.000145) # fixed notation
print(0.0000145) # scientific notation, 6 d.p. in Positron
print(10000) # fixed notation
print(100000) # scientific notation
print(12345.123456) # 3.d.p. |
) * Initial plan * Add test for format_options state change after R option modification Co-authored-by: kv9898 <105025148+kv9898@users.noreply.github.com> * Improve error handling in test_format_options_state_change Co-authored-by: kv9898 <105025148+kv9898@users.noreply.github.com> * Remove unnecessary move keyword in closure Co-authored-by: kv9898 <105025148+kv9898@users.noreply.github.com>
|
Thank you all for this PR! We ask that you sign our Contributor License Agreement before we accept your contribution. You can sign the CLA by posting a comment on this PR saying: I have read the CLA Document and I hereby sign the CLA 2 out of 3 committers have signed the CLA. |
The test has been added in 9569779! |
|
recheck |
| // max_integral_digits = (10 + 5).max(1) = 15 | ||
| // large_num_digits = (10 - 5).max(0) = 5 | ||
| // small_num_digits = (10 + 6).max(1) = 16 | ||
| assert_eq!(format_options.max_integral_digits, 15); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like tests are failing with: thread 'test_format_options_state_change' (32147) panicked at crates/ark/tests/data_explorer.rs:3160:9: assertion `left == right` failed left: 5 right: 15
Any idea @kv9898 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only fails for R 4.2 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDK, this might be related: wch/r-source@7f20c19
There's something special about scipen. Because digits are being correctly evaluated. Perhaps we can't use get_option for this, and instead we bay need to evaluate getOption from the R interpreter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only fails for R 4.2 🤔
Thx for checking that out. Because the test worked for me with R 4.5.2:
E:\ark [scipen ≡] > cargo test -p ark --test data_explorer
Compiling ark v0.1.220 (E:\ark\crates\ark)
warning: unused import: `std::time::Duration`
--> crates\ark\src\fixtures\dummy_frontend.rs:7:5
|
7 | use std::time::Duration;
| ^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unused_imports)]` (part of `#[warn(unused)]`) on by default
warning: `ark` (lib) generated 1 warning (run `cargo fix --lib -p ark` to apply 1 suggestion)
Finished `test` profile [unoptimized + debuginfo] target(s) in 14.20s
Running tests\data_explorer.rs (target\debug\deps\data_explorer-27782ce78c282663.exe)
running 40 tests
test test_export_with_sort_order ... ok
test test_basic_mtcars ... ok
test test_column_labels_edge_cases ... ok
test test_column_labels ... ok
test test_column_labels_haven_compatibility ... ok
test test_column_labels_missing ... ok
test test_data_explorer_special_values ... ok
test test_data_table_support ... ok
test test_data_update_num_rows ... ok
test test_empty_data_frame_column_profiles ... ok
test test_empty_data_frame_data_values ... ok
test test_empty_data_frame_schema ... ok
test test_export_data ... ok
test test_format_options_state_change ... ok
test test_empty_data_frame_state ... ok
test test_frequency_table ... ok
test test_get_data_values_by_indices ... ok
test test_histogram ... ok
test test_histogram_single_bin_same_values ... ok
test test_invalid_filters_preserved ... ok
test test_live_updates ... ok
test test_matrix_support ... ok
test test_null_counts ... ok
test test_row_names_matrix ... ok
test test_schema_identification ... ok
test test_search_filters ... ok
test test_search_schema_combined_filters ... ok
test test_search_schema_data_type_filters ... ok
test test_search_schema_edge_cases ... ok
test test_search_schema_no_matches ... ok
test test_search_schema_sort_orders ... ok
test test_search_schema_text_filters ... ok
test test_search_schema_text_with_sort_orders ... ok
test test_search_schema_type_sort_orders ... ok
test test_set_membership_filter ... ok
test test_single_row_data_frame_column_profiles ... ok
test test_summary_stats ... ok
test test_tibble_support ... ok
test test_update_data_filters_reapplied ... ok
test test_women_dataset ... ok
test result: ok. 40 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.83sThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I don't have much of a clue. Feel free to edit my code if you find a solution! Wrapping the call in as.integer() seems to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDK, this might be related: wch/r-source@7f20c19
There's something special about scipen. Because digits are being correctly evaluated. Perhaps we can't use get_option for this, and instead we bay need to evaluate
getOptionfrom the R interpreter.
Tried that but sadly it didn't work.
|
Now the tests are passed! |
@juliasilge Ark seems use its own RequireCLA script and does not follow your change in posit-dev/cla-assistant@b7814b8. Note Copilot is not in: Line 52 in 910bb6d
|
dfalbel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
@juliasilge I'm still not fully convinced that R's defaults are better compared to Positron Data explorer defaults.
There's no way though to identify if users have explicitly set scipen and digits, ie, if they are really overriding the defaults.
But this probably something that can be handled on the UI side, ie, I'm assuming that in the front-end side PR we'll allow users to decide if they want data explorer to match R's settings or not.
Co-authored-by: Daniel Falbel <dfalbel@gmail.com>
|
recheck |
|
Can we merge this now? |
|
I'm going to work on this soon, together with the PR to |
Addresses posit-dev/positron#4818, in tandem with posit-dev/positron#11051.
The combined end result looks like (using the code example in the original issue):

This pull request introduces frontend formatting options for the data explorer backend and ensures that these options are provided in backend state replies. The changes also include the logic to calculate formatting options based on the current R session settings.
Frontend formatting options integration:
format_optionsfield of typeOption<FormatOptions>to theBackendStatestruct incrates/amalthea/src/comm/data_explorer_comm.rsto support frontend display customization.RDataExplorer::get_state, the code now includes current formatting options by calling a new helper method.Calculation of formatting options:
current_format_optionsmethod inRDataExplorerto compute formatting options based on the R session'sscipenoption, determining integral digit limits and other display parameters.get_optionfromharpto retrieve R session options necessary for formatting calculations.