Skip to content

45 handling of relative errors#46

Merged
archaeothommy merged 10 commits into
mainfrom
45-handling-of-relative-errors
May 25, 2026
Merged

45 handling of relative errors#46
archaeothommy merged 10 commits into
mainfrom
45-handling-of-relative-errors

Conversation

@Abagna123
Copy link
Copy Markdown
Collaborator

No description provided.

@Abagna123 Abagna123 requested a review from archaeothommy April 14, 2026 15:12
@Abagna123 Abagna123 linked an issue Apr 14, 2026 that may be closed by this pull request
3 tasks
Copy link
Copy Markdown
Owner

@archaeothommy archaeothommy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Abagna123 Maybe it is easier this way to trace down what has to be changed.

In addition to the comments:

  • Look into sapply() for e.g. retrieving the column attribute for each column. For example, any(sapply(colnames(df), is_err_percent)) tells you if at least one column in an ASTR dataset is a relative error.
  • Try to find a way to avoid the loop. The functions we worked on before should provide some good ideas.
  • Provide tests and examples.

Comment thread R/ASTR_error_conversion.R Outdated
# Find all error columns
error_cols <- get_error_columns(df)

if (length(error_cols) == 0) {
Copy link
Copy Markdown
Owner

@archaeothommy archaeothommy Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Abagna123 This test will never fail because error_cols will always include the ID columns.
Before this check, remove columns with absolute errors. Look into the functions "is_err_abs()" and "is_err_percent()" for this.

Comment thread R/ASTR_error_conversion.R Outdated
# Check if error is relative (has % units)
err_unit <- units::deparse_unit(df[[err_col]])

if (!err_unit %in% c("%", "atP", "wtP")) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Abagna123: This is not necessary, see comment above.

Comment thread R/ASTR_error_conversion.R Outdated
# Set units to match the concentration column
units(df[[err_col]]) <- units(df[[base_name]])

# Update ASTR_class to mark as absolute error
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Abagna123 We won't introduce additional classes for this distinction. There are already functions for testing this, see comment above.

Comment thread R/ASTR_error_conversion.R Outdated

# Find all error columns (both regular and absolute-marked)
error_cols <- get_error_columns(df, "ASTR_error")
abs_error_cols <- get_cols_with_ac_class(df, "ASTR_error_absolute")
Copy link
Copy Markdown
Owner

@archaeothommy archaeothommy Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Abagna123 This class does not exist.

Comment thread R/ASTR_error_conversion.R Outdated
next
}

# Check if error is absolute (same unit as concentration)
Copy link
Copy Markdown
Owner

@archaeothommy archaeothommy Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Abagna123 You pass only absolute columns into the loop. Therefore, is this test really necessary?

@Abagna123
Copy link
Copy Markdown
Collaborator Author

@archaeothommy I removed the /100 and *100 factors from rel_to_abs() and abs_to_rel() because the units package in R already handles percentage scaling automatically via the UDUNITS database.

When read_ASTR() reads a column with % in the name (e.g., SiO2_errSD%), it stores the numeric value (e.g., 4.3) with a % unit attached. The UDUNITS database defines % as 0.01. As a result, when I extract as.numeric() on that column, it returns 0.043 (already divided by 100).

Thomas Rose added 3 commits May 25, 2026 11:37
* handling edge cases
* inclusion in parsing workflow
* update implementation documentation and vignette
@archaeothommy
Copy link
Copy Markdown
Owner

Updated the functions to

  • Handle conversions of analytical precisions for unitless values (e.g. isotope ratios)
  • Preserve classes of error columns. to return ASTR objects.
  • Rename error columns to indicate the correct type of error
  • Included actual examples

Found and fixed a bug that is_err_abs() also returned relative errors because of an imprecise regex (@nevrome)

@archaeothommy archaeothommy merged commit 5547d27 into main May 25, 2026
1 of 4 checks passed
@archaeothommy archaeothommy deleted the 45-handling-of-relative-errors branch May 25, 2026 18:31
@nevrome
Copy link
Copy Markdown
Collaborator

nevrome commented May 27, 2026

Nice - good job, folks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handling of relative errors

3 participants