rickhelmus / patRoon

Workflow solutions for mass-spectrometry based non-target analysis.
https://rickhelmus.github.io/patRoon/
GNU General Public License v3.0
58 stars 17 forks source link

Suggestion: 'conc'-column in newProject() #102

Closed LeonSaal closed 4 months ago

LeonSaal commented 5 months ago

Hi Rick,

for feature regression analysis, a conc-column is needed in the analysisInfo, however there is no possibility to add this information in the newProject()-GUI. I'm creating a PR to add this functionality.

Related to this issue: I find it a bit confusing, that generateAnalysisInfo takes _e.g. normconcs, concs as arguments i.e. the plural of the resulting columns _normconc and conc in the returned data.frame.

This discrepancy can also be found in the handbook for feature regression analysis:

...The conditions are specified in the conc column of the analysis information, for instance:


# obtain analysis information as usual, but add some experimental parameters of interest (dilution, time etc).
# The blanks are set to NA, whereas the standards are set to increasing levels.

anaInfo <- generateAnalysisInfo(paths = patRoonData::exampleDataPath(), groups = c(rep("solvent", 3), rep("standard", 3)), blanks = "solvent", concs = c(NA, NA, NA, 1, 2, 3))


> If no experimental conditions are available for a particular analysis then the `conc` value should be NA...

Personally, I would prefer, if the arguments to `genrateAnalysisInfo` would be equal to the resulting columns.

Kind regards,

Leon
rickhelmus commented 4 months ago

Hi Leon,

Thanks again for the PR, I added some comments there.

For generateAnalysisInfo(): this function is now very old and therefore comes from a time where things weren't as consistent, perhaps. While I would be happy to change the argument names to be singular, I personally think it's quite minor and not really worth the potential breakage of all existing scripts using generateAnalysisInfo(). But I will keep it in mind if any major changes to this function will be done in the future.

Thanks,

rickhelmus commented 4 months ago

PR merged!