Andy bennett/684 continue adding datapipeline #91

andyb96247 · 2020-08-27T16:53:52Z

Data pipeline read functionality should be complete in its current form. The github workflow still doesn't work.

Resolves ScottishCovidResponse/SCRCIssueTracking#684

…684-continue-adding-datapipeline

peter-t-fox · 2020-09-10T07:41:26Z

.github/workflows/eera.yml

    - name: Compile
      env:
              CC: ${{ matrix.config.compiler }}
              CXX: ${{ matrix.config.compilerpp }}
      run: |
+        source .venv/bin/activate


I think the reason why the CI is currently failing for macOS is that you do this source .venv/bin/activate for both the macOS and Ubuntu builds, but the virtual environment has only been set up for Ubuntu in the "Data pipeline Dependencies" step.

I guess you would need to split the compile into two: one for MacOS, and one for Ubuntu, to fix it.

That's a fair point. I'm not familiar enough with MacOS to know quite what to do to fix without needing to go starting looking things up. Hopefully it isn't too difficult.

peter-t-fox · 2020-09-10T07:46:08Z

src/CMakeLists.txt

@@ -6,7 +6,7 @@ find_package(GSL REQUIRED)
 set(PRE_CONFIGURE_FILE "Git.cpp.in")
 set(POST_CONFIGURE_FILE "${CMAKE_CURRENT_BINARY_DIR}/Git.cpp")
 include(${CMAKE_SOURCE_DIR}/cmake/git_watcher.cmake)
-add_library(git SHARED ${POST_CONFIGURE_FILE})
+add_library(git STATIC ${POST_CONFIGURE_FILE})


Why the change to STATIC?

The cmake for libdatapipeline.a is not building with '-fPIC' so needs STATIC here. A better fix might be to add '-fPIC'/SHARED there and keep SHARED here.

peter-t-fox · 2020-09-10T08:34:43Z

src/ModelCommon.cpp

+	// be used in this sum. The value of obs.cases[0][0] will be -1, so unlikely to show
+	// much?
+
+	// for (unsigned int region = 0; region < obs.cases.size() - 1; ++region) {


Good spot. Looking at the code, I think you are right: it is incorrect. I've added an issue to track this here: ScottishCovidResponse/SCRCIssueTracking#751

peter-t-fox · 2020-09-10T09:05:03Z

src/IO.cpp

@@ -240,22 +208,64 @@ ObservationsForInference ReadInferenceObservations(const std::string& configDir,
    return observations;
 }

+PredictionConfig ReadPredictionConfig(const std::string& configDir, int index, Utilities::logging_stream::Sptr log, const CommonModelInputParameters& commonParameters)


I don't understand the split between ReadPredictionConfig and ReadLocalPredictionConfig. It seems as though all of the data in both functions comes from local sources, rather than the pipeline, so why the split?

The difference is that the "Local" function always occurs with or without the data pipeline (there is a call to it from both) since these values are always read from the local files. ReadPredictionConfig only gets called in the non data pipeline, with the equivalent IOdatapipeline::ReadPredictionConfig called for the data pipeline cases.

I just wanted to avoid some duplication. This is similar for the LocalInference function.

peter-t-fox · 2020-09-10T10:40:55Z

test/unit/ArgParserTest.cpp

+    EXPECT_EQ(parse.getArgs().datapipeline_path, "my_config.yaml");
+}
+
+TEST(AnArgumentParser, TerminatesIfNoDataPipelinePath)


This test doesn't do what is intended. You can confirm this by altering the body of the test to:

const char* _test_args_default[] = {"exe", "-c", "foo"}; EXPECT_EXIT( ArgumentParser(3, _test_args_default), ExitedWithCode(1), "");

The test still passes, when in fact it should fail, given that a path "foo" has been supplied .

The reason that the original test passes is because the model structure and run mode are missing. These are required arguments, and their absence causes the observed termination, not the absence of the data pipeline path.

The correct implementation would be:

const char* _test_args_default[] = {"exe", "-m", "inference", "-s", "original", "-c"}; EXPECT_EXIT( ArgumentParser(6, _test_args_default), ExitedWithCode(1), "");

peter-t-fox · 2020-09-10T10:48:07Z

test/unit/DataPipelineTest.cpp

+    const std::string out_dir = std::string(ROOT_DIR)+"/outputs";
+    Utilities::logging_stream::Sptr logger = std::make_shared<Utilities::logging_stream>(out_dir);
+
+    EXPECT_ANY_THROW(IO::IOdatapipeline idp2("../test/datapipeline/parameters.ini", "", logger, "NoValidPath.yaml"););


I'd suggest using the ROOT_DIR macro to set up this path, rather than a relative path. With the relative path, the test will not work if it is run from somewhere other than the build directory.

Same comment applies elsewhere in this file.

Changed this

peter-t-fox · 2020-09-10T10:50:03Z

test/unit/DataPipelineTest.cpp

 }
+
+namespace {


I think this code has been duplicated in a few places. If you need it multiple times, I'd suggest moving it into the Utilities.h file and including it from there.

I think the "operator<<" functions were there just for debugging so I've removed them in both places.

peter-t-fox · 2020-09-10T11:09:24Z

CMakeLists.txt

@@ -4,7 +4,7 @@ set(PROJECT_NAME Covid19EERAModel)

 project(${PROJECT_NAME} VERSION 0.10.0 LANGUAGES CXX)

-set(CMAKE_CXX_STANDARD 11)
+set(CMAKE_CXX_STANDARD 17)


What's the reason for requiring C++17 support?

My concern would be that only relatively up-to-date compilers support it: people working on older systems may struggle to build this code if we require 17. But if we need a feature from it, then it's okay.

The datapipeline.hh file is currently using std::is_same_v which is a C++17 feature.

andyb96247 added 18 commits August 10, 2020 15:31

Still working

260a261

First array loaded + git URI

5af4aba

More data pipeline arrays

b64fe23

Adding more data + tests

7c846b3

Merge remote-tracking branch 'origin/dev-pipeline' into andy-bennett/…

fa85135

…684-continue-adding-datapipeline

Still working

57723cf

Inference Priors using data pipeline

eda1a28

Inference config pipelined + checks

d5adf3a

Prediction config ported

d47fe72

Action tweak

8570e4c

Action tweak

93e66b1

Fixed for latest pipeline API

ab90abe

Some additional documentation

4f509e3

Doc update

5711255

Action tweak

a161257

Action tweak

3e0a7da

Action tweak

d5cb3db

Action tweak

186e789

andyb96247 requested a review from a team August 27, 2020 16:53

andyb96247 added 2 commits August 30, 2020 11:22

Bug fix

b9434e1

Change to CI

23f9df8

peter-t-fox requested changes Sep 10, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Andy bennett/684 continue adding datapipeline #91

Andy bennett/684 continue adding datapipeline #91

andyb96247 commented Aug 27, 2020 •

edited

Loading

peter-t-fox Sep 10, 2020

andyb96247 Sep 10, 2020

peter-t-fox Sep 10, 2020

andyb96247 Sep 10, 2020

peter-t-fox Sep 10, 2020

peter-t-fox Sep 10, 2020

andyb96247 Sep 10, 2020

peter-t-fox Sep 10, 2020

andyb96247 Sep 10, 2020

peter-t-fox Sep 10, 2020

andyb96247 Sep 10, 2020

peter-t-fox Sep 10, 2020

andyb96247 Sep 10, 2020

peter-t-fox Sep 10, 2020

andyb96247 Sep 10, 2020

		}

		namespace {

Andy bennett/684 continue adding datapipeline #91

Are you sure you want to change the base?

Andy bennett/684 continue adding datapipeline #91

Conversation

andyb96247 commented Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyb96247 commented Aug 27, 2020 •

edited

Loading