Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/0712 model base class #713

Merged
merged 38 commits into from Jul 18, 2019
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
523c11f
template constructor to base model
Jul 5, 2019
0e7723b
Merge branch 'develop' of https://github.com/stan-dev/cmdstan into fe…
Jul 6, 2019
162ef7b
remove unused template parameter in command
Jul 6, 2019
2590273
update includes for command
Jul 6, 2019
e9f3f43
include model_base
mitzimorris Jul 8, 2019
fc9ef34
updated makefile rules
mitzimorris Jul 8, 2019
de431f8
checkpointing makefile
mitzimorris Jul 8, 2019
bc7ee52
Merge branch 'feature/0712-model-base-class' of https://github.com/st…
Jul 8, 2019
cde80e7
remove spurious blank line
mitzimorris Jul 8, 2019
1af3eee
change link to compile; same definition
mitzimorris Jul 8, 2019
845ec86
model headers at compile, not link
mitzimorris Jul 9, 2019
9c66a19
cannot make manual on circle ci - docs not in bookdown
mitzimorris Jul 9, 2019
de37126
revert to develop version of submodule stan
mitzimorris Jul 9, 2019
60521aa
Merge branch 'feature/0712-model-base-class' of https://github.com/st…
Jul 9, 2019
79b133c
use makefile variable for precompiled headers
mitzimorris Jul 9, 2019
81de06f
Merge branch 'feature/0712-model-base-class' of https://github.com/st…
Jul 9, 2019
6d0dd9f
remove .d generation to make executable
Jul 9, 2019
51f39b8
Merge branch 'develop' of https://github.com/stan-dev/cmdstan into fe…
Jul 9, 2019
0f6b9d5
Merge branch 'feature/0712-model-base-class' of https://github.com/st…
mitzimorris Jul 9, 2019
7e3b9d3
makefile removes main.o as part of clean-all
mitzimorris Jul 9, 2019
81f659c
make/program better readiblity; cleans out .cpp and .o files
mitzimorris Jul 9, 2019
506b25c
use pathsubst for CMDSTAN_MAIN_O
mitzimorris Jul 14, 2019
7e316f0
Merge branch 'develop' of https://github.com/stan-dev/cmdstan into fe…
mitzimorris Jul 14, 2019
323c2f0
changes per code review comments
mitzimorris Jul 15, 2019
fde50d2
fixed unit tests
mitzimorris Jul 15, 2019
c56eb1f
tweaked makefiles, deps generated only for unit tests, not models
mitzimorris Jul 16, 2019
d403f0c
use var CMDSTAN_MAIN_O everywhere
mitzimorris Jul 16, 2019
b22052b
makefile cleanup per code review request
mitzimorris Jul 17, 2019
865116e
makefile - task compile_info
mitzimorris Jul 17, 2019
c4efe06
need mpi header
mitzimorris Jul 17, 2019
1bede8a
another attempt at mpi headers
mitzimorris Jul 17, 2019
9404d6f
probably not a fix
mitzimorris Jul 18, 2019
4e42237
Merge branch 'develop' of https://github.com/stan-dev/cmdstan into fe…
mitzimorris Jul 18, 2019
e0d08d5
fix comment, try different link order
mitzimorris Jul 18, 2019
0e76075
fix mpi test
mitzimorris Jul 18, 2019
18f23f4
add back in test for manual
mitzimorris Jul 18, 2019
2157e92
simplify test for manual
mitzimorris Jul 18, 2019
655e93f
makefile comment
mitzimorris Jul 18, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 0 additions & 17 deletions .circleci/config.yml

This file was deleted.

15 changes: 10 additions & 5 deletions make/program
Expand Up @@ -2,6 +2,7 @@
# Models (to be passed through stanc)
##
CMDSTAN_MAIN ?= src/cmdstan/main.cpp
CMDSTAN_MAIN_O ?= src/cmdstan/main.o
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the patsubst command to derive from CMDSTAN_MAIN the value for CMDSTAN_MAIN_O.


STAN_TARGETS = $(patsubst %.stan,%$(EXE),$(wildcard $(patsubst %$(EXE),%.stan,$(MAKECMDGOALS))))

Expand All @@ -23,22 +24,26 @@ ifneq ($(findstring allow_undefined,$(STANCFLAGS)),)
$(STAN_TARGETS) examples/bernoulli/bernoulli$(EXE) $(patsubst %.stan,%$(EXE),$(wildcard src/test/test-models/*.stan)) : CXXFLAGS_PROGRAM += -include $(USER_HEADER)
endif


%.hpp : %.stan bin/stanc$(EXE)
@echo ''
@echo '--- Translating Stan model to C++ code ---'
$(WINE) bin/stanc$(EXE) $(STANCFLAGS) --o=$@ $<

%.d: %.hpp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definition here overrides the one included in the top makefile which pulls in math/make/dependencies. Could you please document that you do this here on purpose. The alternative is to not include the dependencies from math which we may not need actually for cmdstan?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing the override.
we need the dependencies rule.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok...once you do that, then the model_header.o dependency can be build. The only downside is that then you will build the dependency thing for the stan model hpp file... which wastes 1s of compile time as it anyway gets recreated with any model update...but ok; still a win.


.PRECIOUS: %.hpp
%$(EXE) : %.hpp $(CMDSTAN_MAIN) $(LIBSUNDIALS) $(MPI_TARGETS)
%$(EXE) : %.hpp $(CMDSTAN_MAIN_O) $(LIBSUNDIALS) $(MPI_TARGETS)
@echo ''
@echo '--- Linking C++ model ---'
$(LINK.cpp) $(CXXFLAGS_PROGRAM) -include $< $(CMDSTAN_MAIN) $(LDLIBS) $(LIBSUNDIALS) $(MPI_TARGETS) $(OUTPUT_OPTION)
@echo '--- Compiling, linking C++ code ---'
cp $*.hpp $*.cpp; \
$(LINK.cpp) $(CXXFLAGS_PROGRAM) -c -o $*.o $*.cpp ; \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh.. this is less ideal, I think. You can make the compiler interpret the hpp file just like a c++ file with the -x c++ option. So I would suggest to replace 38-41 with this:

        $(COMPILE.cpp) $(CXXFLAGS_PROGRAM) -x c++ -o $*.o $<
        $(LINK.cpp) $(LDLIBS) $(LIBSUNDIALS) $(MPI_TARGETS) $(CMDSTAN_MAIN_O) $*.o $(OUTPUT_OPTION)

This way you pickup the right things for compilation (first line) and for linking (second line). Moreover, you do not need to do this copying and removing business.

$(CXX) -include $(LDLIBS) $(LIBSUNDIALS) $(MPI_TARGETS) -o $@ $(CMDSTAN_MAIN_O) $*.o; \
rm $*.cpp $*.o



##
# Dependencies file
# Dependencies file for precompiled header
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment is right - these are dependency files for everything. Line 53 includes the one from the model_header file.

##
ifneq (,$(STAN_TARGETS))
$(patsubst %$(EXE),%.d,$(STAN_TARGETS)) : DEPTARGETS += -MT $(patsubst %.d,%$(EXE),$@) -include $< -include $(CMDSTAN_MAIN)
Expand Down
3 changes: 2 additions & 1 deletion makefile
Expand Up @@ -151,7 +151,7 @@ build-mpi: $(MPI_TARGETS)
@echo '--- boost mpi bindings built ---'

.PHONY: build
build: bin/stanc$(EXE) bin/stansummary$(EXE) bin/print$(EXE) bin/diagnose$(EXE) $(LIBSUNDIALS) $(MPI_TARGETS)
build: bin/stanc$(EXE) bin/stansummary$(EXE) bin/print$(EXE) bin/diagnose$(EXE) $(LIBSUNDIALS) $(MPI_TARGETS) $(CMDSTAN_MAIN_O) $(STAN)src/stan/model/model_header.d
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The model_header.d should just be removed here, I think. At least this caused errors for me and I don't quite see the need for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can stay now once you remove the %.d override, but please verify.

@echo ''
@echo '--- CmdStan v$(CMDSTAN_VERSION) built ---'

Expand Down Expand Up @@ -182,6 +182,7 @@ clean-manual:

clean-all: clean clean-deps clean-libraries clean-manual
$(RM) -r bin
$(RM) -r src/cmdstan/main.o
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to delete $(CMDSTAN_MAIN_O) here and not directly name the file defined in the variable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be adjusted?

$(RM) $(wildcard $(STAN)src/stan/model/model_header.hpp.gch)

##
Expand Down
10 changes: 8 additions & 2 deletions src/cmdstan/command.hpp
Expand Up @@ -18,6 +18,7 @@
#include <stan/io/stan_csv_reader.hpp>
#include <stan/io/ends_with.hpp>
#include <stan/io/json/json_data.hpp>
#include <stan/model/model_base.hpp>
#include <stan/services/diagnose/diagnose.hpp>
#include <stan/services/optimize/bfgs.hpp>
#include <stan/services/optimize/lbfgs.hpp>
Expand Down Expand Up @@ -49,6 +50,12 @@
#include <vector>
#include <memory>


// forward declaration for function defined in another translation unit
stan::model::model_base& new_model(stan::io::var_context& data_context,
unsigned int seed,
std::ostream* msg_stream);

namespace cmdstan {

#ifdef STAN_MPI
Expand Down Expand Up @@ -80,7 +87,6 @@ namespace cmdstan {
static int hmc_fixed_cols = 7; // hmc sampler outputs columns __lp + 6


template <class Model>
int command(int argc, const char* argv[]) {
stan::callbacks::stream_writer info(std::cout);
stan::callbacks::stream_writer err(std::cout);
Expand Down Expand Up @@ -137,7 +143,7 @@ namespace cmdstan {

unsigned int random_seed = dynamic_cast<u_int_argument*>(parser.arg("random")->arg("seed"))->value();

Model model(*var_context, random_seed, &std::cout);
stan::model::model_base& model = new_model(*var_context, random_seed, &std::cout);

write_stan(sample_writer);
write_model(sample_writer, model.model_name());
Expand Down
6 changes: 3 additions & 3 deletions src/cmdstan/main.cpp
@@ -1,11 +1,11 @@
#include <cmdstan/command.hpp>
#include <stan/services/error_codes.hpp>
#include <boost/exception/diagnostic_information.hpp>
#include <cmdstan/command.hpp>
#include <boost/exception/diagnostic_information.hpp>
#include <boost/exception_ptr.hpp>

int main(int argc, const char* argv[]) {
try {
return cmdstan::command<stan_model>(argc,argv);
return cmdstan::command(argc,argv);
} catch (const std::exception& e) {
std::cout << e.what() << std::endl;
return stan::services::error_codes::SOFTWARE;
Expand Down