search

Meredith-Lab / volcalc

volcalc: Calculate Volatility of Chemical Compounds
https://meredith-lab.github.io/volcalc/
Other
5 stars 1 forks source link

Are amines double-counted? #49

Closed Aariq closed 1 year ago

Aariq commented 1 year ago

primary, secondary, and tertiary amines are already included in calc_vol() with coefficients and then this line also adds a coefficient for amines. Doesn't that double-count amines?

https://github.com/Meredith-Lab/volcalc/blob/9e7250a9462224659766546efc1937236c07908b/R/calc_vol.R#L93C26-L93C26

KristinaRiemer commented 1 year ago

We originally counted all four types of amines to compare between the three subtypes and the overall count. At some point we talked about how it looked like the primary amines and just plain old amines are the same and we should get rid of the latter, but I guess we just still haven't actually done that?

Aariq commented 1 year ago

Ok, good to know. I'll hold off on fixing this until after we get through currently open PRs. The next PR will hopefully include more tests for get_fx_groups() that will catch this kind of thing.

@ledfordmarshall—FYI,numbers might be slightly off for amines.

KristinaRiemer commented 1 year ago

There's a little conversation about this in the manuscript, linked to the "Amine" in the coefficients table.

Aariq commented 1 year ago

On second thought, maybe we should deal with this and release a v1.0.2 before big breaking changes get merged. Any preference?