From ce77f713e0e380d17d1801af8ef40a68b87f107f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Villemot?= <sebastien@dynare.org>
Date: Fri, 10 Jul 2020 14:55:28 +0200
Subject: [PATCH] PAC MCE: fix incorrect detection of the target variable

The detection of the target EC variable to be used when constructing the
forward-looking expectation variable is rather fragile.

When the PAC model is written with an (non-)optimizing share of agents,
restrict the identification of the target variable to the optimizing
expression, to minimize the risk of wrong identification.

By the way, add a few comments, and a small simplification.

(cherry picked from commit 16f9168fda3a0c8e4b1d162aae9ccbfe537fb1ec)
---
 src/DynamicModel.cc | 16 ++++++++++++----
 src/ExprNode.cc     |  5 ++++-
 src/ExprNode.hh     |  3 +++
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/src/DynamicModel.cc b/src/DynamicModel.cc
index 5bbca050..415fe5d5 100644
--- a/src/DynamicModel.cc
+++ b/src/DynamicModel.cc
@@ -4523,11 +4523,9 @@ DynamicModel::getPacTargetSymbId(const string &pac_model_name) const
   for (auto &equation : equations)
     if (equation->containsPacExpectation(pac_model_name))
       {
-        pair<int, int> lhs(-1, -1);
         set<pair<int, int>> lhss;
         equation->arg1->collectDynamicVariables(SymbolType::endogenous, lhss);
-        lhs = *lhss.begin();
-        int lhs_symb_id = lhs.first;
+        int lhs_symb_id = lhss.begin()->first;
         int lhs_orig_symb_id = lhs_symb_id;
         if (symbol_table.isAuxiliaryVariable(lhs_symb_id))
           try
@@ -4537,7 +4535,17 @@ DynamicModel::getPacTargetSymbId(const string &pac_model_name) const
           catch (...)
             {
             }
-        return equation->arg2->getPacTargetSymbId(lhs_symb_id, lhs_orig_symb_id);
+        auto barg2 = dynamic_cast<BinaryOpNode *>(equation->arg2);
+        assert(barg2);
+        auto [optim_share_index, optim_part, non_optim_part, additive_part]
+          = barg2->getPacOptimizingShareAndExprNodes(lhs_symb_id, lhs_orig_symb_id);
+        /* The algorithm for identifying the target is fragile. Hence, if there
+           is an optimization part, we restrict the search to that part to
+           avoid wrong results. */
+        if (optim_part)
+          return optim_part->getPacTargetSymbId(lhs_symb_id, lhs_orig_symb_id);
+        else
+          return equation->arg2->getPacTargetSymbId(lhs_symb_id, lhs_orig_symb_id);
       }
   return -1;
 }
diff --git a/src/ExprNode.cc b/src/ExprNode.cc
index ca91b743..4f81d325 100644
--- a/src/ExprNode.cc
+++ b/src/ExprNode.cc
@@ -5034,8 +5034,11 @@ BinaryOpNode::getPacTargetSymbIdHelper(int lhs_symb_id, int undiff_lhs_symb_id,
     {
       int id = datatree.symbol_table.getUltimateOrigSymbID(it.first);
       if (id == lhs_symb_id || id == undiff_lhs_symb_id)
-        found_lagged_lhs = true;
+        found_lagged_lhs = true; // This expression contains the (lagged) LHS
       if (id != lhs_symb_id && id != undiff_lhs_symb_id)
+        /* The first variable that is not the (lagged) LHS is a
+           candidate for the target. FIXME: What happens if there are several
+           such variables? The detection order is not meaningful… */
         if (target_symb_id < 0)
           target_symb_id = it.first;
     }
diff --git a/src/ExprNode.hh b/src/ExprNode.hh
index 9f8bcd15..0ce14aee 100644
--- a/src/ExprNode.hh
+++ b/src/ExprNode.hh
@@ -483,6 +483,9 @@ public:
   virtual int PacMaxLag(int lhs_symb_id) const = 0;
 
   //! Get the target variable of the PAC model
+  /* The algorithm is rather crude. For a BinaryOpNode, it inspects both
+     arguments. If one of the argument contains the (lagged) LHS, the first
+     variable that is not the (lagged) LHS is returned as the target */
   virtual int getPacTargetSymbId(int lhs_symb_id, int undiff_lhs_symb_id) const = 0;
 
   virtual expr_t undiff() const = 0;
-- 
GitLab