Re: [stan] updating arg_init. The test for this is in CmdStan. (#750)

[Bob Carpenter]

Hang on! You steamrolled that one through without
any code review. Otherwise, I might not mind, but I
have two objections to this pull request:

https://github.com/stan-dev/stan/pull/750/files

1. We have a hard-coded path in src/stan/gm/arguments/arg_init.hpp:

+ _good_value = "../src/test/test-models/test_model.init.R";

2. And it appears to be an R dependency!

I don't think we should have either hardcoded paths or R dependencies
anywhere.

On Jul 7, 2014, at 11:01 PM, Daniel Lee <notifi...@github.com> wrote:

> Merged #750.
>
> —
> Reply to this email directly or view it on GitHub.
>

[Daniel Lee]

My fault --- it was holding up CmdStan.

I completely agree that this shouldn't be hardcoded, but not sure how to handle this. Thoughts?

>
> --

> Reply to this email directly or view it on GitHub.
>

--
You received this message because you are subscribed to the Google Groups "stan development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to stan-dev+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Bob Carpenter]

Actually, I now realize that's just a data file in
R format, not an actual R dependency. But objection (1)
still holds.

- Bob

>> --

>> Reply to this email directly or view it on GitHub.
>>
>

[Bob Carpenter]

I can't even tell what it's doing. Why is there a hardcoded
path in one of our API header files?

We can put this back on the issue discussion if you want.
Could you reopen it?

- Bob

[Daniel Lee]

Let me create a new item for reviewing the argument structure. Now that I have a better handle on it, I think I want to walk through it.

[Daniel Lee]

New issue created:
https://github.com/stan-dev/stan/issues/756

[Michael Betancourt]

The path is used only for tests and points to the generic directory for test models.

An artifact of having the code generate it’s own tests (the argument code will

create configurations using that file as a init and make sure CmdStan runs

without failing).  I can’t see a way around it…

[Bob Carpenter]

If it's only used for the tests, then it should be configured
in the tests. The crudest solution would be to just drop
a mutable member variable in the class where it's defined and
set it from the tests.

But I'd be in favor of going even further if it's not too much
work and just moving whatever functionality auto-generates the
tests to the test code and out of the API.

Also, what's the point of having the code generate its own tests?
Or more specifically, when will a self-generated test fail?

- Bob

[Michael Betancourt]

It’s testing the validity of the every combination of valid/invalid arguments,
so it’s really more of an integration test. Anyone wants to code that by hand be my guest.

On Jul 8, 2014, at 9:20 PM, Bob Carpenter <ca...@alias-i.com> wrote:

> If it's only used for the tests, then it should be configured
> in the tests. The crudest solution would be to just drop
> a mutable member variable in the class where it's defined and
> set it from the tests.
>
> But I'd be in favor of going even further if it's not too much
> work and just moving whatever functionality auto-generates the
> tests to the test code and out of the API.
>
> Also, what's the point of having the code generate its own tests?
> Or more specifically, when will a self-generated test fail?
>
> - Bob
>
>
>
> On Jul 8, 2014, at 3:24 PM, Michael Betancourt <betan...@gmail.com> wrote:
>
>> The path is used only for tests and points to the generic directory for test models.
>> An artifact of having the code generate it's own tests (the argument code will
>> create configurations using that file as a init and make sure CmdStan runs

>> without failing). I can't see a way around it...

[Bob Carpenter]

I'm asking what would cause the test to fail. No point in running
tests if they are guaranteed to succeed by design.

My concern is that we don't have a spec for what valid/invalid
arguments are other than the code, and that's what's generating
the tests.

If the spec is so complex that we can't write the tests by hand,
I think that signals another problem in the interface. This is
one of the reasons I want to modularize rather than to pile more
things into the same command --- it drastically cuts down on
the combinatorics.

[Daniel Lee]

On Tue, Jul 8, 2014 at 4:48 PM, Bob Carpenter <ca...@alias-i.com> wrote:

I'm asking what would cause the test to fail.   No point in running
tests if they are guaranteed to succeed by design.

After this pull request, they fail when the files (init file, data file, CmdStan executable program) can't be found in the location specified.

(Before this pull request, they didn't fail when files couldn't be found.)

The executable is run and parts of the output are checked against known behavior.

 

My concern is that we don't have a spec for what valid/invalid
arguments are other than the code, and that's what's generating
the tests.

That is also my concern, but I'm more concerned with the ability of interface writers to configure this thing properly and maintain it as Stan chugs along.

[Michael Betancourt]

After this pull request, they fail when the files (init file, data file, CmdStan executable program) can't be found in the location specified.

(Before this pull request, they didn't fail when files couldn't be found.)

The executable is run and parts of the output are checked against known behavior.

It will also fail is Stan doesn’t use the configuration to pick up the file correctly, for example.

My concern is that we don't have a spec for what valid/invalid
arguments are other than the code, and that's what's generating
the tests.

That is also my concern, but I'm more concerned with the ability of interface writers to configure this thing properly and maintain it as Stan chugs along.

I don’t see how the test can be modularized given that the arguments can all iterate with each other.