Skip to content

fix: reduce buildScoreCache example size and clean up export_abnFit example - #261

Open
LeonidasZhak wants to merge 1 commit into
furrer-lab:mainfrom
LeonidasZhak:fix/254-reduce-example-sample-size
Open

LeonidasZhak wants to merge 1 commit into
furrer-lab:mainfrom
LeonidasZhak:fix/254-reduce-example-sample-size

Conversation

@LeonidasZhak

Copy link
Copy Markdown

Summary

This PR addresses issue #254 by making the following documentation improvements:

Changes

  1. R/build_score_cache.R (line 283): Reduced N from 1e6 to 100 in the buildScoreCache() example. 1 million observations is excessive for a simple example; 100 is sufficient to demonstrate the workflow.

  2. R/export.R (lines 152-161): Cleaned up export_abnFit() example:

    • Removed unnecessary data(ex1.dag.data) call (data is available via lazy loading)
    • Removed unnecessary library(abn) call (not needed in examples)
    • Fixed missing distributions: added b4 = "binomial" and b5 = "binomial" to match the 10 variables in ex1.dag.data
  3. man/buildScoreCache.Rd and man/export_abnFit.Rd: Updated Rd files to match roxygen source

Validation

  • Verified ex1.dag.data has 10 columns: b1, p1, g1, b2, p2, b3, g2, b4, b5, g3
  • Verified the fixed mydists list now has 10 matching entries
  • Verified the diff is small and focused (4 files, 8 insertions, 14 deletions)

Related Issues

…xample

- Reduce N from 1e6 to 100 in buildScoreCache() example (issue furrer-lab#254)
- Remove unnecessary data() call from export_abnFit() example
- Fix missing distributions in export_abnFit() example (b4, b5)
@matteodelucchi

Copy link
Copy Markdown
Contributor

@learbuehrer could you take over the review of this?

@learbuehrer

Copy link
Copy Markdown
Collaborator

Thanks for the fix and for the pull request.

I’m perfectly happy with change 1 — reducing N from 1e6 to 100 makes sense for the small example.

For change 2, I agree that b4 and b5 need to be added. However, it is also necessary to keep mydists in the same order as the variables in ex1.dag.data. The proposed version is slightly inconsistent in this respect, as g2 appears before b3, while the data are ordered as:

b1, p1, g1, b2, p2, b3, g2, b4, b5, g3

The updated mydists should thus be:

mydists <- list(
  b1 = "binomial", p1 = "poisson", g1 = "gaussian",
  b2 = "binomial", p2 = "poisson", b3 = "binomial",
  g2 = "gaussian", b4 = "binomial", b5 = "binomial",
  g3 = "gaussian")

This would both include all 10 variables and preserve the ordering of ex1.dag.data.

There is also another issue I noticed when testing the example. buildScoreCache() runs successfully with max.parents = 0 and max.parents = 1, but fails with max.parents = 2:

Error: task 13 failed - "solve(): solution not found"

So the example currently fails with max.parents = 2. I think this is worth investigating before merging, since the documented example should ideally run successfully with the specified max.parents value.

Thanks again for addressing the documentation issues!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants