You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
At first I also wanted to drop it - but then I realized:
this allows also to use import("matchms", convert = FALSE) to disable automatic conversion of python to R and vice versa, maybe not wrong to disable to speed up things?
Also, in a loop, you can define this reference once, and then in the loop re-use it. Speeds up things.
I would thus keep it (it's anyway for the advanced user).
I think having a function that populates a global variable might be the best way?
I don't think that passing the reference to the imported matchms is a good thing - this implies you could use different versions of matchms in a single session etc., which is not very clean.
But we can figure out how to do this properly in another issue.
What I've seen so far in other packages is that they call the import within each function. It's just not very efficient if you repeatedly do that in a loop. At present the conversion is done on each spectrum individually in such a loop (which is also not that efficient).
I totally agree that using different versions of matchms would be problematic - but here I trust the user - these function should only be for the advanced user, and they, hopefully, know what they are doing.
I am not in favour of a global variable, because that might prevent us from using parallel processing (well, question is also if we would really want to do that...).
Well we can export the variable to the compute cluster, right? Or is that one of the things the biocparallel package can't do? I have the vague memory that there was something like that.
I think having a function that populates a global variable might be the best way?
I don't think that passing the reference to the imported matchms is a good thing - this implies you could use different versions of matchms in a single session etc., which is not very clean.
But we can figure out how to do this properly in another issue.
Originally posted by @hechth in #10 (comment)
The text was updated successfully, but these errors were encountered: