Repository: NREL/OpenStudio (language: python)

Issue:
Schedule:File relative paths
Pull request overview
---------------------

 - Fixes #4865

Currently,

```
OS:External:File,
  {a055bc90-48c7-47a2-9949-425486cded8e}, !- Handle
  External File 1,                        !- Name
  workflow/sample_files/run/in.schedules.csv; !- File Name
```
translates to
```
Schedule:File,
  lighting_interior,                      !- Name
  Fractional,                             !- Schedule Type Limits Name
  C:/OpenStudio/OpenStudio-HPXML/workflow/sample_files/run/in.schedules.csv, !- File Name
  2,                                      !- Column Number
  1,                                      !- Rows to Skip at Top
  8760,                                   !- Number of Hours of Data
  Comma,                                  !- Column Separator
  No,                                     !- Interpolate to Timestep
  60,                                     !- Minutes per Item
  Yes;                                    !- Adjust Schedule for Daylight Savings
```
We want (optionally) "File Name" to be translated to `workflow/sample_files/run/in.schedules.csv`.

~I think we can just use `externalFile.filePath()` being relative/absolute as the switch in ScheduleFile FT. It's absolute when using either (1) `ScheduleFile.new(external_file)`, or (2) `ScheduleFile.new(model, /abs/path)`. It's relative when using `ScheduleFile.new(model, /rel/path)`.~

Looks like `externalFile.filePath` is always FT'd to "File Name" as an absolute path. So, this PR introduces a new bool field "Translate File Name" on the `OS:Schedule:File` object (defaults to false). When false, the old behavior is preserved (the FT translates `externalFile.filePath`. When true, the FT translates `externalFile.fileName` into "File Name".

Add FT tests for checking "File Name" with `scheduleFile.setTranslateFileName(true)`:
- `ScheduleFile.new(externalFile(model, /abs/path))` -> rel path
- `ScheduleFile.new(externalFile(model, /rel/path))` -> rel path
- `ScheduleFile.new(model, /abs/path)` -> abs path
- `ScheduleFile.new(model, /rel/path)` -> rel path

### Pull Request Author

<!--- Add to this list or remove from it as applicable.  This is a simple templated set of guidelines. -->

 - [x] Model API Changes / Additions
 - [x] Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
 - [x] Model API methods are tested (in `src/model/test`)
 - [x] EnergyPlus ForwardTranslator Tests (in `src/energyplus/Test`)
 - [ ] If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
 - [ ] If needed, added VersionTranslation rules for the objects (`src/osversion/VersionTranslator.cpp`)
 - [ ] Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
 - [ ] All new and existing tests passes
 - [ ] If methods have been deprecated, update rest of code to use the new methods

**Labels:**

 - [x] If change to an IDD file, add the label `IDDChange`
 - [ ] If breaking existing API, add the label `APIChange`
 - [ ] If deemed ready, add label `Pull Request - Ready for CI` so that CI builds your PR

### Review Checklist

This will not be exhaustively relevant to every PR.
 - [ ] Perform a Code Review on GitHub
 - [ ] Code Style, strip trailing whitespace, etc.
 - [ ] All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
 - [ ] Labeling is ok
 - [ ] If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
 - [ ] If feature, test running new feature, try creative ways to break it
 - [ ] CI status: all green or justified



Discussion / hints:
@joseph-robertson this is still marked as draft, is more work needed?
> @joseph-robertson this is still marked as draft, is more work needed?

Yes, more work is needed. I need to circle back to this. 

Fix patch:
diff --git a/resources/model/OpenStudio.idd b/resources/model/OpenStudio.idd
index 3d91e263dc5..2541bf85f55 100644
--- a/resources/model/OpenStudio.idd
+++ b/resources/model/OpenStudio.idd
@@ -5111,7 +5111,9 @@ OS:Schedule:File,
        \key Yes
        \key No
        \default Yes
-  A8;  \field Translate File Name
+  A8;  \field Translate File With Relative Path
+       \note "No" means the absolute path of the ExternalFile is resolved and translated to IDF.
+       \note "Yes" means only the filename (relative) is translated
        \type choice
        \default No
        \key Yes
diff --git a/src/energyplus/ForwardTranslator/ForwardTranslateScheduleFile.cpp b/src/energyplus/ForwardTranslator/ForwardTranslateScheduleFile.cpp
index 6bf95504039..22b049411b2 100644
--- a/src/energyplus/ForwardTranslator/ForwardTranslateScheduleFile.cpp
+++ b/src/energyplus/ForwardTranslator/ForwardTranslateScheduleFile.cpp
@@ -40,22 +40,8 @@ namespace energyplus {
         idfObject.setString(openstudio::Schedule_FileFields::ScheduleTypeLimitsName, idfScheduleTypeLimits->name().get());
       }
     }
-
-    path fileName;
-    if (modelObject.translateFileName()) {
-      fileName = toPath(modelObject.externalFile().fileName());
-    } else {
-      fileName = modelObject.externalFile().filePath();
-      if (!exists(fileName)) {
-        LOG(Warn, "Cannot find file \"" << fileName << "\"");
-      } else {
-        // make the path correct for this system
-        fileName = system_complete(fileName);
-      }
-    }
-
     // DLM: this path is going to be in the temp dir, might want to fix it up when saving model temp dir
-    idfObject.setString(openstudio::Schedule_FileFields::FileName, toString(fileName));
+    idfObject.setString(openstudio::Schedule_FileFields::FileName, toString(modelObject.translatedFilePath()));
 
     idfObject.setInt(openstudio::Schedule_FileFields::ColumnNumber, modelObject.columnNumber());
     idfObject.setInt(openstudio::Schedule_FileFields::RowstoSkipatTop, modelObject.rowstoSkipatTop());
diff --git a/src/energyplus/Test/ScheduleInterval_GTest.cpp b/src/energyplus/Test/ScheduleInterval_GTest.cpp
index 9b40e702b2b..13ab2cd2af9 100644
--- a/src/energyplus/Test/ScheduleInterval_GTest.cpp
+++ b/src/energyplus/Test/ScheduleInterval_GTest.cpp
@@ -30,6 +30,7 @@
 #include <utilities/idd/IddEnums.hxx>
 
 #include "../../utilities/core/PathHelpers.hpp"
+#include "../../utilities/core/Filesystem.hpp"
 #include <resources.hxx>
 
 #include <boost/regex.hpp>
@@ -1857,25 +1858,31 @@ TEST_F(EnergyPlusFixture, DISABLED_ForwardTranslator_ScheduleVariableInterval_Da
 }
 
 TEST_F(EnergyPlusFixture, ScheduleFileRelativePath) {
+
+  openstudio::path absoluteScheduleFilePath = resourcesPath() / toPath("model/schedulefile.csv");
+  ASSERT_TRUE(openstudio::filesystem::is_regular_file(absoluteScheduleFilePath));
+  ASSERT_TRUE(absoluteScheduleFilePath.is_absolute());
+
+  openstudio::path curDirPath = boost::filesystem::current_path();
+  openstudio::path relativeScheduleFilePath = openstudio::filesystem::relative(absoluteScheduleFilePath, curDirPath);
+  ASSERT_TRUE(relativeScheduleFilePath.is_relative());
+
   // ScheduleFile(external_file(/abs/path))
   {
     Model model;
     EXPECT_EQ(0u, model.getConcreteModelObjects<ExternalFile>().size());
     EXPECT_EQ(0u, model.getConcreteModelObjects<ScheduleFile>().size());
 
-    path p = resourcesPath() / toPath("model/schedulefile.csv");
-    EXPECT_FALSE(p.is_relative());
-
-    boost::optional<ExternalFile> external_file = ExternalFile::getExternalFile(model, openstudio::toString(p));
+    boost::optional<ExternalFile> external_file = ExternalFile::getExternalFile(model, openstudio::toString(absoluteScheduleFilePath));
     ASSERT_TRUE(external_file);
     ScheduleFile schedule(*external_file);
-    EXPECT_TRUE(schedule.setTranslateFileName(true));
-    EXPECT_TRUE(schedule.translateFileName());
+    EXPECT_TRUE(schedule.setTranslateFileWithRelativePath(true));
+    EXPECT_TRUE(schedule.translateFileWithRelativePath());
     EXPECT_EQ(1u, model.getConcreteModelObjects<ScheduleFile>().size());
     EXPECT_EQ(1u, model.getConcreteModelObjects<ExternalFile>().size());
     ExternalFile externalfile = schedule.externalFile();
     EXPECT_EQ(1u, externalfile.scheduleFiles().size());
-    EXPECT_EQ(openstudio::toString(p.filename()), externalfile.fileName());
+    EXPECT_EQ(openstudio::toString(absoluteScheduleFilePath.filename()), externalfile.fileName());
     EXPECT_TRUE(toPath(externalfile.fileName()).is_relative());
     EXPECT_FALSE(externalfile.filePath().is_relative());
     EXPECT_NE(toPath(externalfile.fileName()), externalfile.filePath());
@@ -1898,19 +1905,16 @@ TEST_F(EnergyPlusFixture, ScheduleFileRelativePath) {
     EXPECT_EQ(0u, model.getConcreteModelObjects<ExternalFile>().size());
     EXPECT_EQ(0u, model.getConcreteModelObjects<ScheduleFile>().size());
 
-    path p = toPath("../../../resources/model/schedulefile.csv");
-    EXPECT_TRUE(p.is_relative());
-
-    boost::optional<ExternalFile> external_file = ExternalFile::getExternalFile(model, openstudio::toString(p));
+    boost::optional<ExternalFile> external_file = ExternalFile::getExternalFile(model, openstudio::toString(relativeScheduleFilePath));
     ASSERT_TRUE(external_file);
     ScheduleFile schedule(*external_file);
-    EXPECT_TRUE(schedule.setTranslateFileName(true));
-    EXPECT_TRUE(schedule.translateFileName());
+    EXPECT_TRUE(schedule.setTranslateFileWithRelativePath(true));
+    EXPECT_TRUE(schedule.translateFileWithRelativePath());
     EXPECT_EQ(1u, model.getConcreteModelObjects<ScheduleFile>().size());
     EXPECT_EQ(1u, model.getConcreteModelObjects<ExternalFile>().size());
     ExternalFile externalfile = schedule.externalFile();
     EXPECT_EQ(1u, externalfile.scheduleFiles().size());
-    EXPECT_EQ(openstudio::toString(p.filename()), externalfile.fileName());
+    EXPECT_EQ(openstudio::toString(relativeScheduleFilePath.filename()), externalfile.fileName());
     EXPECT_TRUE(toPath(externalfile.fileName()).is_relative());
     EXPECT_FALSE(externalfile.filePath().is_relative());
     EXPECT_NE(toPath(externalfile.fileName()), externalfile.filePath());
@@ -1933,17 +1937,14 @@ TEST_F(EnergyPlusFixture, ScheduleFileRelativePath) {
     EXPECT_EQ(0u, model.getConcreteModelObjects<ExternalFile>().size());
     EXPECT_EQ(0u, model.getConcreteModelObjects<ScheduleFile>().size());
 
-    path p = resourcesPath() / toPath("model/schedulefile.csv");
-    EXPECT_FALSE(p.is_relative());
-
-    ScheduleFile schedule(model, openstudio::toString(p));
-    EXPECT_TRUE(schedule.setTranslateFileName(true));
-    EXPECT_TRUE(schedule.translateFileName());
+    ScheduleFile schedule(model, openstudio::toString(absoluteScheduleFilePath));
+    EXPECT_TRUE(schedule.setTranslateFileWithRelativePath(true));
+    EXPECT_TRUE(schedule.translateFileWithRelativePath());
     EXPECT_EQ(1u, model.getConcreteModelObjects<ScheduleFile>().size());
     EXPECT_EQ(1u, model.getConcreteModelObjects<ExternalFile>().size());
     ExternalFile externalfile = schedule.externalFile();
     EXPECT_EQ(1u, externalfile.scheduleFiles().size());
-    EXPECT_EQ(openstudio::toString(p), externalfile.fileName());
+    EXPECT_EQ(openstudio::toString(absoluteScheduleFilePath), externalfile.fileName());
     EXPECT_FALSE(toPath(externalfile.fileName()).is_relative());
     EXPECT_FALSE(externalfile.filePath().is_relative());
     EXPECT_EQ(toPath(externalfile.fileName()), externalfile.filePath());
@@ -1966,19 +1967,16 @@ TEST_F(EnergyPlusFixture, ScheduleFileRelativePath) {
     EXPECT_EQ(0u, model.getConcreteModelObjects<ExternalFile>().size());
     EXPECT_EQ(0u, model.getConcreteModelObjects<ScheduleFile>().size());
 
-    path p = toPath("../../../resources/model/schedulefile.csv");
-    EXPECT_TRUE(p.is_relative());
-
-    ScheduleFile schedule(model, openstudio::toString(p));
-    EXPECT_TRUE(schedule.setTranslateFileName(true));
-    EXPECT_TRUE(schedule.translateFileName());
+    ScheduleFile schedule(model, openstudio::toString(relativeScheduleFilePath));
+    EXPECT_TRUE(schedule.setTranslateFileWithRelativePath(true));
+    EXPECT_TRUE(schedule.translateFileWithRelativePath());
     EXPECT_EQ(1u, model.getConcreteModelObjects<ScheduleFile>().size());
     EXPECT_EQ(1u, model.getConcreteModelObjects<ExternalFile>().size());
     ExternalFile externalfile = schedule.externalFile();
     EXPECT_EQ(1u, externalfile.scheduleFiles().size());
-    EXPECT_EQ(openstudio::toString(p), externalfile.fileName());
+    EXPECT_EQ(openstudio::toString(relativeScheduleFilePath), externalfile.fileName());
     EXPECT_TRUE(toPath(externalfile.fileName()).is_relative());
-    EXPECT_TRUE(externalfile.filePath().is_relative());
+    EXPECT_TRUE(externalfile.filePath().is_relative()) << externalfile.filePath();
     EXPECT_EQ(toPath(externalfile.fileName()), externalfile.filePath());
 
     ForwardTranslator ft;
diff --git a/src/model/ScheduleFile.cpp b/src/model/ScheduleFile.cpp
index db57dc1e1a6..5649fb17b29 100644
--- a/src/model/ScheduleFile.cpp
+++ b/src/model/ScheduleFile.cpp
@@ -233,14 +233,14 @@ namespace model {
       return csvFile;
     }
 
-    bool ScheduleFile_Impl::translateFileName() const {
-      boost::optional<std::string> value = getString(OS_Schedule_FileFields::TranslateFileName, true);
+    bool ScheduleFile_Impl::translateFileWithRelativePath() const {
+      boost::optional<std::string> value = getString(OS_Schedule_FileFields::TranslateFileWithRelativePath, true);
       OS_ASSERT(value);
       return openstudio::istringEqual(value.get(), "Yes");
     }
 
-    bool ScheduleFile_Impl::isTranslateFileNameDefaulted() const {
-      return isEmpty(OS_Schedule_FileFields::TranslateFileName);
+    bool ScheduleFile_Impl::isTranslateFileWithRelativePathDefaulted() const {
+      return isEmpty(OS_Schedule_FileFields::TranslateFileWithRelativePath);
     }
 
     /* FIXME!
@@ -386,22 +386,36 @@ namespace model {
     }*/
     }
 
-    bool ScheduleFile_Impl::setTranslateFileName(bool translateFileName) {
+    bool ScheduleFile_Impl::setTranslateFileWithRelativePath(bool translateFileWithRelativePath) {
       bool result = false;
-      if (translateFileName) {
-        result = setString(OS_Schedule_FileFields::TranslateFileName, "Yes");
+      if (translateFileWithRelativePath) {
+        result = setString(OS_Schedule_FileFields::TranslateFileWithRelativePath, "Yes");
       } else {
-        result = setString(OS_Schedule_FileFields::TranslateFileName, "No");
+        result = setString(OS_Schedule_FileFields::TranslateFileWithRelativePath, "No");
       }
       OS_ASSERT(result);
       return result;
     }
 
-    void ScheduleFile_Impl::resetTranslateFileName() {
-      const bool result = setString(OS_Schedule_FileFields::TranslateFileName, "");
+    void ScheduleFile_Impl::resetTranslateFileWithRelativePath() {
+      const bool result = setString(OS_Schedule_FileFields::TranslateFileWithRelativePath, "");
       OS_ASSERT(result);
     }
 
+    openstudio::path ScheduleFile_Impl::translatedFilePath() const {
+      if (translateFileWithRelativePath()) {
+        return toPath(externalFile().fileName());
+      }
+      openstudio::path filePath = externalFile().filePath();
+      if (!exists(filePath)) {
+        LOG(Warn, "Cannot find file \"" << filePath << "\"");
+      } else {
+        // make the path correct for this system
+        filePath = system_complete(filePath);
+      }
+      return filePath;
+    }
+
   }  // namespace detail
 
   ScheduleFile::ScheduleFile(const ExternalFile& externalfile, int column, int rowsToSkip)
@@ -503,12 +517,12 @@ namespace model {
     return getImpl<detail::ScheduleFile_Impl>()->csvFile();
   }
 
-  bool ScheduleFile::translateFileName() const {
-    return getImpl<detail::ScheduleFile_Impl>()->translateFileName();
+  bool ScheduleFile::translateFileWithRelativePath() const {
+    return getImpl<detail::ScheduleFile_Impl>()->translateFileWithRelativePath();
   }
 
-  bool ScheduleFile::isTranslateFileNameDefaulted() const {
-    return getImpl<detail::ScheduleFile_Impl>()->isTranslateFileNameDefaulted();
+  bool ScheduleFile::isTranslateFileWithRelativePathDefaulted() const {
+    return getImpl<detail::ScheduleFile_Impl>()->isTranslateFileWithRelativePathDefaulted();
   }
 
   /* FIXME!
@@ -585,12 +599,16 @@ unsigned ScheduleFile::addTimeSeries(const openstudio::TimeSeries& timeSeries) {
     getImpl<detail::ScheduleFile_Impl>()->resetAdjustScheduleforDaylightSavings();
   }
 
-  bool ScheduleFile::setTranslateFileName(bool translateFileName) {
-    return getImpl<detail::ScheduleFile_Impl>()->setTranslateFileName(translateFileName);
+  bool ScheduleFile::setTranslateFileWithRelativePath(bool translateFileWithRelativePath) {
+    return getImpl<detail::ScheduleFile_Impl>()->setTranslateFileWithRelativePath(translateFileWithRelativePath);
+  }
+
+  void ScheduleFile::resetTranslateFileWithRelativePath() {
+    getImpl<detail::ScheduleFile_Impl>()->resetTranslateFileWithRelativePath();
   }
 
-  void ScheduleFile::resetTranslateFileName() {
-    getImpl<detail::ScheduleFile_Impl>()->resetTranslateFileName();
+  openstudio::path ScheduleFile::translatedFilePath() const {
+    return getImpl<detail::ScheduleFile_Impl>()->translatedFilePath();
   }
 
   /// @cond
diff --git a/src/model/ScheduleFile.hpp b/src/model/ScheduleFile.hpp
index 2295236f6d7..9d95b07a27e 100644
--- a/src/model/ScheduleFile.hpp
+++ b/src/model/ScheduleFile.hpp
@@ -86,9 +86,9 @@ namespace model {
 
     boost::optional<CSVFile> csvFile() const;
 
-    bool translateFileName() const;
+    bool translateFileWithRelativePath() const;
 
-    bool isTranslateFileNameDefaulted() const;
+    bool isTranslateFileWithRelativePathDefaulted() const;
 
     //@}
     /** @name Setters */
@@ -124,14 +124,17 @@ namespace model {
 
     /* FIXME! unsigned addTimeSeries(const openstudio::TimeSeries& timeSeries); */
 
-    bool setTranslateFileName(bool translateFileName);
+    bool setTranslateFileWithRelativePath(bool translateFileWithRelativePath);
 
-    void resetTranslateFileName();
+    void resetTranslateFileWithRelativePath();
 
     //@}
     /** @name Other */
     //@{
 
+    // Depending on the value of 'Translate File With Relative Path', returns an absolute or a relative path
+    openstudio::path translatedFilePath() const;
+
     //@}
    protected:
     /// @cond
diff --git a/src/model/ScheduleFile_Impl.hpp b/src/model/ScheduleFile_Impl.hpp
index 901f36693fd..5c125645bf0 100644
--- a/src/model/ScheduleFile_Impl.hpp
+++ b/src/model/ScheduleFile_Impl.hpp
@@ -88,9 +88,9 @@ namespace model {
 
       boost::optional<CSVFile> csvFile() const;
 
-      bool translateFileName() const;
+      bool translateFileWithRelativePath() const;
 
-      bool isTranslateFileNameDefaulted() const;
+      bool isTranslateFileWithRelativePathDefaulted() const;
 
       //@}
       /** @name Setters */
@@ -125,14 +125,16 @@ namespace model {
       // ensure that this object does not contain the date 2/29
       virtual void ensureNoLeapDays() override;
 
-      bool setTranslateFileName(bool translateFileName);
+      bool setTranslateFileWithRelativePath(bool translateFileWithRelativePath);
 
-      void resetTranslateFileName();
+      void resetTranslateFileWithRelativePath();
 
       //@}
       /** @name Other */
       //@{
 
+      openstudio::path translatedFilePath() const;
+
       //@}
      protected:
      private:


Test patch:
diff --git a/src/model/test/ScheduleInterval_GTest.cpp b/src/model/test/ScheduleInterval_GTest.cpp
index aa43f33b750..bdee273366e 100644
--- a/src/model/test/ScheduleInterval_GTest.cpp
+++ b/src/model/test/ScheduleInterval_GTest.cpp
@@ -320,14 +320,18 @@ TEST_F(ModelFixture, ScheduleFile) {
   schedule3.resetAdjustScheduleforDaylightSavings();
   EXPECT_TRUE(schedule3.isAdjustScheduleforDaylightSavingsDefaulted());
 
-  EXPECT_FALSE(schedule3.translateFileName());
-  EXPECT_TRUE(schedule3.isTranslateFileNameDefaulted());
-  EXPECT_TRUE(schedule3.setTranslateFileName(true));
-  EXPECT_TRUE(schedule3.translateFileName());
-  EXPECT_FALSE(schedule3.isTranslateFileNameDefaulted());
-  schedule3.resetTranslateFileName();
-  EXPECT_FALSE(schedule3.translateFileName());
-  EXPECT_TRUE(schedule3.isTranslateFileNameDefaulted());
+  EXPECT_FALSE(schedule3.translateFileWithRelativePath());
+  EXPECT_TRUE(schedule3.isTranslateFileWithRelativePathDefaulted());
+  EXPECT_EQ(externalfile->filePath(), schedule3.translatedFilePath());
+
+  EXPECT_TRUE(schedule3.setTranslateFileWithRelativePath(true));
+  EXPECT_TRUE(schedule3.translateFileWithRelativePath());
+  EXPECT_FALSE(schedule3.isTranslateFileWithRelativePathDefaulted());
+  EXPECT_EQ(toPath(externalfile->fileName()), schedule3.translatedFilePath());
+
+  schedule3.resetTranslateFileWithRelativePath();
+  EXPECT_FALSE(schedule3.translateFileWithRelativePath());
+  EXPECT_TRUE(schedule3.isTranslateFileWithRelativePathDefaulted());
 
   // shouldn't create a new object
   boost::optional<ExternalFile> externalfile2 = ExternalFile::getExternalFile(model, openstudio::toString(p));