search

rformassspectrometry / MetaboCoreUtils

Core utilities for metabolomics.
https://rformassspectrometry.github.io/MetaboCoreUtils/index.html
7 stars 6 forks source link

feat: accept NA in countElements; see #61 #62

Closed sgibb closed 11 months ago

sgibb commented 11 months ago

This PR allows NAs in subtractElements and addElements by handling NAs in countElements (problem described in #61 )

subtractElements("H2O", NA) returns "H2O". Or should it return NA?

Unfortunately it introduce a strange behaviour for containsElements:

containsElements("H2O", NA)
# [1] TRUE

IMHO containsElements("H2O", NA) should return FALSE or NA.

@jorainer, @michaelwitting any suggestion for the expected behaviour?

jorainer commented 11 months ago

IMO the following would make sense:

containsElements("H2O", NA) should return NA substractElement("H2O", NA) returns NA_character_ containsElements("H2O", "") should return FALSE (same as e.g. containsElements("H2O", "Z"))

with the current PR you're also reporting TRUE for containsElements("H2O", "Z")... thus, it always returns TRUE even if the provided element does not exist...

sgibb commented 11 months ago

containsElements("H2O", NA) should return NA substractElement("H2O", NA) returns NA_character_

done!

containsElements("H2O", "") should return FALSE (same as e.g. containsElements("H2O", "Z")) with the current PR you're also reporting TRUE for containsElements("H2O", "Z")... thus, it always returns TRUE even if the provided element does not exist...

This wasn't introduced by this PR but is a bug that was/is already present. I add an issue #63 .

IMHO we could close #61 with this PR and fix #63 in another one.