Skip to content
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

invalid elements/isotopes in countElements #64

Closed
sgibb opened this issue Aug 4, 2023 · 3 comments
Closed

invalid elements/isotopes in countElements #64

sgibb opened this issue Aug 4, 2023 · 3 comments
Labels
question Further information is requested

Comments

@sgibb
Copy link
Member

sgibb commented Aug 4, 2023

While looking at #63 I recognize that countElements silently drops invalid elements if at least one valid element was found because of its very complex regular expression (that matches mostly valid element names):

countElements("C1Z")
# $C1Z
# C 
# 1 
countElements("Foo")
# $Foo
# F 
# 1 

In contrast if the invalid element is given without any valid one:

countElements("Z")

# $Z
# named integer(0)
#
# Warning message:
# In (function (xx, rr)  :
#  The following names are not valid and are dropped: 1

Because of that containsElements("C1", c("Z", "C1Z")) yield c(TRUE, TRUE) instead of c(FALSE, FALSE).
While it is easy to test if countElements returns an integer(0) (e.g. for "Z") it is not possible to test for dropped elements if there is at least one valid element (e.g. for "C1Z", we could test pasteElements(countElements("C1Z")) == "C1Z"), which would be error prone and slow).

Shouldn't we return NA if at least one element name is invalid (instead of currently silently dropping it or throw a warning if there is at least one valid element)?

@jorainer
Copy link
Member

jorainer commented Aug 8, 2023

Agree - reporting NA in these cases would make sense (and is in line with the general NA handling in R).

@jorainer
Copy link
Member

jorainer commented Aug 8, 2023

I'm perfectly fine with your solution! Does that mean we need to revert the fix in #62 ? in any rate, please update version and add NEWS and then merge. Thanks!

@sgibb
Copy link
Member Author

sgibb commented Aug 8, 2023

No #62 is still needed (we could maybe drop the is (anyNA(x)) check in .sum_elements but adductFormula doesn't generate a named list and will fail).

Closed by #65

@sgibb sgibb closed this as completed Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants