search

cbg-ethz / MC-CBN

MC-CBN performs large-scale inference on conjunctive Bayesian networks
GNU General Public License v2.0
4 stars 3 forks source link

igraph, foreach, R.utils, MASS, relations: as "Imports" (and with importFrom) rathen than Depends #8

Closed rdiaz02 closed 2 years ago

rdiaz02 commented 2 years ago

Right now, there is this line in the DESCRIPTION file

Depends: igraph, foreach, R.utils, MASS, relations

But that means that any package that wants to use MC-CBN also must have a Depends with those packages (see, for example, https://stackoverflow.com/a/8638902). Would it be possible to have those packages in "Imports", with the corresponding importFrom (or, in the worst case, import) in the NAMESPACE file, to avoid having the list of packages that are loaded grow? Thanks!

sposadac commented 2 years ago

Thanks!

rdiaz02 commented 2 years ago

It still fails, with errors like could not find function "relation_incidence" or similar for as.relation and transitive_reduction. It is necessary to add relations in the NAMESPACE file. (The change in commit https://github.com/cbg-ethz/MC-CBN/commit/096b8cfaae8bfa2416a2f0601ea21bf75f923cef only changed DESCRIPTION but nothing from the package relations was actually imported, which is what is specified in the NAMESPACE file).

One option is having , in NAMESPACE a line like

import(relations)

but that imports maybe too much. I personally prefer to import only what I need. I just run the example referred to in https://github.com/cbg-ethz/MC-CBN/issues/9 , and it works fine if I have this in the NAMESPACE file

importFrom(relations, as.relation, transitive_reduction, relation_incidence)

(note that for other functions in mccbn additional functions from relations might need to be imported; I did not check this).

sposadac commented 2 years ago

Thanks for the feedback. I performed additional checks and these imports seem to suffice.

rdiaz02 commented 2 years ago

Thanks a lot!