Skip to content

support for calculating one indexed entry in a node instruction or instruction list, and related changes. - #71

Merged
perrydv merged 2 commits into
mainfrom
instr-indexing
Nov 5, 2026
Merged

perrydv merged 2 commits into
mainfrom
instr-indexing

Conversation

@perrydv

@perrydv perrydv commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

This PR makes substantial changes to the instruction and instruction list system and provides new methods for using singletons from within one of those.

  1. instrList_nClass replaces nList(instr_nClass) as the type for node lists. It holds information such as the cumulative number of nodes across the instructions it holds.
  2. Its R-list analogue is RinstrList. Previously we had Rlist_Rinstr, which seems no longer needed and so is removed.
  3. instr_nClass is extended to hold precalculated information such as strides (based on the indexing ranges of the instructions, not the whole model variable) and related info for quickly determining the indexing that matches "nodes number i" within the instruction.
  4. Note that actual use of model$calculate(nodes[i]) will only exist in nimble2, because it requires "keyword processing" on instances. Here we have the underlying new methods: calculate_indexed, calculateDiff_indexed, getLogProb_indexed, simulate_indexed, getParam_indexed and getBound_indexed.
  5. Calls like model$calculate(instrList) or model$calculate_indexed(instrList, i) can now be compiled in an nFunction or nClass. Previously this was only feasible from nimble2. This makes nimbleModel more fully featured and should be useful.
  6. Note that supporting compilation led to an exercise in looking at what the argument names should be, and making them match for the uncompiled versions and the matchDef entry in the opDefs for the compiled versions. Specifically many args named instr were changed to instrList. Uncompiled, they are polymorphic and handle either, but compiled they must be instrList objects, so I named them that.
  7. There is a slight inconsistency to keep in mind and discuss later about getParam and getBound. These take an instr, not an instrList. But getParam_indexed and getBound_indexed take an instrList to be fully general. I decided to just note that and move on for now.
  8. The simulate "runners" (iteration functions) are now the same as for calculate etc.

There are two bug fixes:

  • Lifted nodes that use only some of their loop indices, e.g. sqrt(s2[j]) lifted from y[i, j] ~ dnorm(0, var = s2[j]), read the wrong idx slot. They were NA uncompiled and gave -Inf compiled. The loop indices now come from loopIndexingRule rather than the declaration's context. A regression test is included, and noted in comments near relevant changes.
  • R calc_5_generic used the wrong slot range for a matrix fourth range.

Tests

  • New: test-instrSteps.R.
  • test-nimbleModel.R now has the regression test for the bug noted above. It also has a new test for compiling all the model operations.

Other

  • .Rbuildignore now excludes a local include -> inst/include symlink used with devtools::load_all() (but the link itself is not in the repo)

@perrydv

perrydv commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

@paciorek Let me know if you want to review this or if I should go ahead and merge.

@paciorek

paciorek commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

I just skimmed things. A few comments:

  1. We also need values(model, nodes[i]) for initializeModel to check for NAs.
  2. I see you decided not to get rid of the "_ord" vs. without "_ord" distinction. Fine by me.
  3. In the C++, the duplication of the NIDX checks in the switch, e.g., that there are two cases that say "NIDX == 1" seems odd. But I didn't try to understand it...
  4. Are we changing the API to calculate_indexed(nodes, j) (from calculate(nodes[j]))or is that just where things stand at the moment?

@perrydv

perrydv commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the comments.

  1. Good point. I will make that a separate piece of work. values() uses nodes as an arbitrary block of a variable (not really nodes per se), so it does not go through the "instruction" machinery.
  2. Yeah, given that we decided to keep all the runners otherwise, it was easier just to keep them for now.
  3. These are C++ compile-time checks avoid compiling runners that will never be used for particular declarations. If a declaration has NIDX indices, then the relevant runners are either those with the same number of indices without matp cases or fewer indices with matp cases. So the two NIDX == 1 are for calc_1_seq_ and calc_1_mat_, which would only be needed for a single-indexed declaration.
  4. In nimbleModel, it will be calculate_indexed(nodes, j) (or something like that, name could be discussed). In nimble2, we would support calculate(nodes[j]) using the keyword processing system. At least that was my working idea. I guess with more work we could support calculate(nodes[j]) in nimbleModel directly, so let's discuss. I think that would be turned into calculate_indexed(nodes, j), so it would build on rather than replace the current PR.

@paciorek

Copy link
Copy Markdown
Contributor

We are probably just using language differently, but values(nodes[i]) seems to me that it would use the same machinery as we have now. We need to figure out what the i'th node in an instrList is and then insert indices to extract from the block (just as with simulate(nodes[i]) we are inserting into said block.

@perrydv

perrydv commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Best for a discussion.

* Steps toward support for values(nodes[i]) support

* updates to back-end implementations for values(model, nodes[i])
@perrydv

perrydv commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Per discussion with @paciorek I am merging this.

@perrydv
perrydv merged commit ebabea8 into main Nov 5, 2026
4 checks passed
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