Contributing New Functions to Stan (dev guide page)

Developers
1

Hi all,

I’ve started going about writing a new function for Stan because

  • I want something for extending ICAR’s beyond what Mitzi’s code can easily do and the current options are very slow
  • I’m sick of having to explain to people that I’m not stan-dev, I’m just pushy*, so I’m taking the first step towards doing something about it.
  • Structured procrastination is my new jam.

As I’m probably the simplest programmer you’re likely to have trying to do this and I’m implementing the simplest program ( I want sparse quadratic forms y = v’Qv, which even I can differentiate), @betanalpha suggested I point out things I find missing in the doc. Obviously, the caveat here is there are probably doc pages I’m not seeing, or things I’m reading too fast. I’m also a very bad programmer. But in the event that this is useful for other people, here are my feelings so far (more will come as feelings develop).

  • Broadly speaking, the Contributing New Functions to Stan page is excellent, but it is not obvious to find from mc-stan.org. (You need to go click to the Developer Process overview and then find it in the menu on the right. It could be more obvious.

  • Similarly, to find the How To Write Unit Tests page you literally need to click on the button to find the hidden topics. Perhaps it would be better for the developer page on mc-stan.org to have an index of wikis separated by topics. This could also be linked in the Contributing New Funtions To Stan page.

  • Some stuff is a bit unclear. For example, the asserts that are commonly on the top of functions (are the argument dimensions correct, is the matrix positive definite etc) present two challenges.
    – The first is that it’s not clear what’s there or where it is (eg the check_size_match.hpp function is in math/prim/scal/ even though its obvious use case is checking dimensions, which are multivariate. It makes sense after you find it.)
    – The second is that some of the @throw doxygen comments are a bit odd. For example, the string for prim/mat/fun/dot_self.hpp says “@throw std::domain_error If v is not vector dimensioned.” while the code for check_vector() just calls the function invalid_argument(), which may through that exception, but it’s not obvious.

  • For the unit testing, I don’t have the foggiest what the TEST( arg, arg) macro (I guess it’s a macro) does and I don’t know where to find the information. I can pattern match, but it’s really only a guess. Also how to run them would be nice. I worked out hat it’s

python runTests.py test/unit/math/prim/mat/fun/dot_self_test.cpp

  • I am not yet at a point to do model tests, but some more doc would probably be useful there too.

  • It would be super-nice to have a more obvious link to the Stan Math library doc on arxiv and maybe a paragraph of “sense checking”. For instance Mike sent me, for computing the reverse mode AD of f(x) that it should be:

adjoints_in [vector of size dim(x)] = J^{T} [matrix of size dim(x) x dim(f) ] * adjoints_out [ vector of size dim(f) ]

  • The key thing there that was useful for me that was harder to read in the ninety-something page doc is exactly what dimension everything should be!

  • There are also some unclear things about memory management for the AD that may only be relevant because I’m reading matrix stuff. For example in rev/mat/fun/multiply.hpp there’s a comment that says

     * It is critical for the efficiency of this object
     * that the constructor create new varis that aren't
     * popped onto the var_stack_, but rather are 
     * popped onto the var_nochain_stack_. This is
     * controlled to the second argument to 
     * vari's constructor.
  • This probably means a lot to existing devs, but I couldn’t find any doc on the memory management for varis. This is very much a fringe issue and maybe not best dealt with through docs (rather through venues like the discourse or during PR reviews), but I’m listing everything.

I’ll add more in the thread if more comes up.

*“I Don’t Push, I’m Just Pushy” is my late-career Sinead O’Connor tribute album.

5 Likes
2

When you do go to implement something (after you talk to people and know it’ll make sense to get it in), feel free to post the specific questions on discourse. I was in your shoes recently enough that I think I can answer your questions and try to update doc at the same time.

3

I’ll add a few things here too since I’ve been going through this.

From a high level, I like the Math docs. The Stan docs are a bit more confusing though. I’ve only done simple things like adding linkage for new functions, but it’s not clear to me what things I should be testing in Stan itself (as opposed to Math). I haven’t really ever had to look at the interface codes to make my stuff work (which is nice!).

Broadly speaking, the Contributing New Functions to Stan page is excellent

I agree totally. Stan + Stan-math are largely a very pleasant pieces of software to work with. There are some issues on this page (Contributing New Functions to Stan · stan-dev/stan Wiki · GitHub) though:

  1. The directions in Directories for code make sense to me now that I know what everything is, but I think it needs more words explaining what things are (what is prim? what is rev? what is mix?) stan::math::rev should be stan::math::var? A link to the built Nomad manual would be useful with respect to mix.
  2. src/test/gm/model_specs/compiled/ doesn’t seem to exist (presumably this was in Stan?)
  3. src/test/unit-agrad-rev/functions/ doesn’t exist (presumably this was in Math?)
  4. make test-unit doesn’t work on the Math library or the Stan library
  5. test/gm doesn’t exist (is this src/test/gm from Stan?)
  6. The Model Tests blurb seems really important, but I had a lot of trouble parsing it (still not sure I understand it – there seem to be a lot of tests there for Math functions, but a lot of Math function tests also seem to be missing [or I could be missing them])
  7. Everything in Creating a C++ Function for Stan down is useful, but it feels like a second version of the stuff I just read (I like the 2nd version better, honestly).

The second is that some of the @throw doxygen comments are a bit odd

Agreed. It’s annoying to burrow through the error checks and find out exactly what error is being thrown (and could lead to outdated docs if that underlying error changes)

For the unit testing, I don’t have the foggiest what the TEST( arg, arg)

I agree. Maybe some Stan-specific info on how the Google unit test stuff is used could be useful. It’s been annoying enough to me that I don’t want to go anywhere near it (EXPECT_THROW/EXPECT_NO_THROW consuming errors). Maybe I should just RTFM, but it’s too many layers removed from the fun stuff for me to really want to stress myself out over it.

4

My own feeling about these docs is that they’re way too scattered. We’ve probably said everything multiple times in various places but it’s hard to consolidate. At the same time, there’s only so much room at the top of our doc and other web pages. Any organizational hints as to how to fix these problems would be appreciated!

Nope. Not even close on either count. We have programmers who’ve only ever touched R. The matrix functions introduce tricky memory management issues for the operands.

If you really want to procrastinate, work on the web site! It’s just another GitHub repo.

The Wiki’s super easy to edit. The problem we run into is where to put all these things. We should at least have this one in the top-level directory.

These aren’t technically asserts. assert() is something built into C/C++ that we don’t use. We (try to) throw exceptions with meaningful messages that get reported back to the user.

This gets particularly tricky to doc as the same thing goes for just about every source code file. Partly you just have to look at other files like the ones you’re using. Or we could write up some doc on all the tests. One thing that needs to be done is generalizing all the tests to vectors and matrices—the coverage is currently spotty.

The doxygen doc is written for users of the function, not coders. We explicitly don’t want the doxygen doc to contain implementation details (except about high-level algorithmic issues that users need to know).

Yes, it’s a macro. It’s from googletest: GitHub - google/googletest: GoogleTest - Google Testing and Mocking Framework They have lots of doc on how it all works. It’s a pretty extensive package and we use a lot of it and we’re not going to just doc it all again. But feel free to include a link from one or more places you think would be helpful.

Just add these things where you think they should go. It’s a wiki.

Not sure what you mean here. Did you want a pointer to Giles’s papers on matrix autodiff?

Definitely something we should add. I thought the arXiv paper covered that, but maybe not. The no_chain stack is just a stack of variables whose chain() methods are not called during autodiff. So you can build up variables to hold results and instead of calling their chain() method, do the work in another vari’s chain method. The reason this is so useful is that it converts a ton of virtual function calls through the autodiff stack (basically interpreted code) into a single function (essentially compiled code).

5

Please just go ahead and add to the doc. I take it you know what these are and the other cross-cutting classification is scal/arr/mat

There is no stan::math::rev, so that’s probably right

Stan doesn’t use Nomad. Do you mean there’s something in the doc that’d be useful? We really need to write that forward-mode doc, but I keep getting distracted.

Would you mind fixing that enumerated list of issues tof things that don’t exist?

I don’t even know what the Model Tests blurb is from memory.

Many people come in and want to write intro guides, so if there are two intros, they should be consolidated. That can involve replacing major bits of what currently look like the main entry. I always find it super confusing to get multiple versions of things.

I’m a bad person to go over this stuff now since I’ve been here since day one and all of this has grown up around me, so it’s very hard to see it from a newcomer’s perspective.

6

I’m not comfortable editing the developer process wiki for a project where I’m not part of the dev team! That way madness lies.

7

Would you mind fixing that enumerated list of issues tof things that don’t exist?

Yeah, sure thing. I’ll be back at the office Thursday.

I’m not comfortable editing the developer process wiki for a project where I’m not part of the dev team! That way madness lies.

I’ve been nervous to make edits for the same reason, but if it’s two unfamiliar people editing I can just blame you if I mess things up :P.

8

There’s a principle here. It would be different if it was a git repo and I could do a PR in, but as it is I think it’s fairly ludicrous for someone who isn’t a developer to edit the developer guidelines.

9

The GitHub wikis are Git repos underneath. We can’t do PRs but there is each edit is a commit, we have the full history, and we can revert. Edit. We can fix any problems introduced.

1 Like
10

Someone quickly add Dan to the dev team!

2 Likes
11

Madness as in waste of time? It’s a Wiki, so you can’t really break anything anyone depends on.

Are you worried you’ll say something wrong? I think there’s a much better chance you’ll just clarify.

12

My view is that once I’ve made it through the full process of contributing
a function to Stan then i am “qualified” to edit the wiki. Before that
would be premature.

I’m very much of the "just because you can, doesn’t mean you should"
school.