Skip to content

remove uses of the nimble namespace, as we move towards nimble2 - #63

Merged
perrydv merged 8 commits into
masterfrom
remove-nimble-namespace-references
Oct 23, 2026
Merged

perrydv merged 8 commits into
masterfrom
remove-nimble-namespace-references

Conversation

@perrydv

@perrydv perrydv commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

I am not sure if this is ready to merge, but let's see if it passes testing and any input from @paciorek.

This simply comments-out the use of nf_changeNimKeywords, pending discussion of where that should be done. (See #60).

I think calc_dmnormAltParams and calc_dwishAltParams will both work as nCompiler::nFunction. However full testing will really come from uses in nimble2.

@paciorek

Copy link
Copy Markdown
Contributor

Ok I did some work to address some of the test failures. This was done with a bit of duct tape and chewing gum to get things working in terms of transitioning to nC and nMul, and now using nCompiler:::nf_changeKeywords.

There are still some modelValues test failures plus a failure with test-setupMargNodes caused by the keyword changes inserting dexp_nCompiler but that seemingly not fully functional.

@perrydv

perrydv commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Evidently the dexp_nCompiler idea was a stub for something similar to dexp_nimble. Whereas R's dexp only uses rate parameterization, dexp_nimble also supports scale, presumably for compatibility with the BUGS language, which uses scale. So the question here is whether supporting both parameterizations should live in nCompiler or in nimbleModel? At first glance it seems like it should live in nimbleModel (or even nimble2, but we have the distribution definitions in nimbleModel, so I think there makes more sense).

@paciorek

Copy link
Copy Markdown
Contributor

yeah I think nimbleModel makes sense as a home for it. I suppose one could argue that various distributions could be used in nFunction code and maybe it should be in nCompiler, but if we think of it as a "user-provided" distribution where the "user" is nimbleModel/us, then nimbleModel makes sense.

@perrydv

perrydv commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

@paciorek This branch is now passing tests. I ended up keeping the feature from nimble of supporting a scale parameterization for dexp and I did that through a keyword change to dexp_nComp. We have the flexibility to discuss that in the future. I realized there are other distributions that we will want to consider moving from nCompiler to nimbleModel, such as dconstraint, which is really a BUGS language trick rather than a proper distribution. But the goal here was to remove all uses of nimble's namespace. In addition this work prompted me to improve nCompiler so that there are compileInfo fields that predefined classes can use to get some necessary content (depends that creates an Rcpp depends attribute in C++ so that a package such as nimbleModel can put itself on the C++ compiler's -I inclusion paths, and also HpostCode). Those changes are now used here so that nimbleModel's generate-predefined process is much simpler.

I am happy with this branch now, if you want to review it.

@paciorek

Copy link
Copy Markdown
Contributor

Looks good. One thing was that on quick glance it looked like the parameterization for dexp_nComp differs from dexp_nimble?

So you will merge this and then do the styler pass?

@perrydv

perrydv commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Can you say what you mean? Both are dexp_<name>(x, rate, scale, log). dexp_nComp handles rate vs scale a little differently because the underlying C++ from the R math library uses scale, and in nimble we could sometimes end up inverting scale to rate and then again back to scale. But I think the parameterizations and behaviors should match, unless I'm missing your point.

@perrydv

perrydv commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

I will go ahead with the merge and then the styler formatting soon, since the dexp_nComp issue seems simple and could be changed later.

@perrydv
perrydv merged commit 133fd53 into master Oct 23, 2026
4 checks passed
@perrydv
perrydv deleted the remove-nimble-namespace-references branch October 23, 2026 16:52
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.

2 participants