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

Schedule:File relative paths #4879

Merged
merged 40 commits into from
Nov 2, 2023
Merged

Schedule:File relative paths #4879

merged 40 commits into from
Nov 2, 2023

Conversation

joseph-robertson
Copy link
Collaborator

@joseph-robertson joseph-robertson commented May 9, 2023

Pull request overview

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

  • Model API Changes / Additions
  • Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
  • Model API methods are tested (in src/model/test)
  • 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:

  • 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

@joseph-robertson joseph-robertson added Enhancement Request Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels May 9, 2023
@joseph-robertson joseph-robertson self-assigned this May 9, 2023
@jmarrec
Copy link
Collaborator

jmarrec commented Jun 30, 2023

@joseph-robertson this is still marked as draft, is more work needed?

@joseph-robertson
Copy link
Collaborator Author

@joseph-robertson this is still marked as draft, is more work needed?

Yes, more work is needed. I need to circle back to this.

@joseph-robertson joseph-robertson marked this pull request as ready for review October 27, 2023 20:21
Comment on lines 5114 to 5118
A8; \field Translate File Name
\type choice
\default No
\key Yes
\key No
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the name is clear. Should have a \note probably as well.

Not a huge fan of having a default rather than hard-coded required param. I know this saves on needing to do VT, and the above has a default, so I guess it's up to you to make it required or not.

Maybe one of these two?

Suggested change
A8; \field Translate File Name
\type choice
\default No
\key Yes
\key No
A8; \field Translate File With Absolute Path
\note "Yes" means the absolute path of the ExternalFile is resolved and translated to IDF.
\note "No" means only the filename (relative) is translated
\type choice
\default Yes
\key Yes
\key No

Suggested change
A8; \field Translate File Name
\type choice
\default No
\key Yes
\key No
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
\key No

Comment on lines 44 to 54
path fileName;
if (modelObject.translateFileName()) {
fileName = toPath(modelObject.externalFile().fileName());
} else {
// make the path correct for this system
filePath = system_complete(filePath);
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);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider putting this logic in ScheduleFile directly, that'd be much easier to debug if something goes awry. And much easier to add a model test for it too.

openstudio::path ScheduleFile::translatedFilePath() const {
  if (translatedFileName()) {
    return toPath(modelObject.externalFile().fileName());
  }
   openstudio::path filePath = modelObject.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;
}

(If externalFile.filePath doesn't exist, maybe it should throw?)

@jmarrec
Copy link
Collaborator

jmarrec commented Oct 31, 2023

Proposed some changes in #5010 @joseph-robertson

@joseph-robertson joseph-robertson added this to the OpenStudio SDK 3.7.0 milestone Oct 31, 2023
@jmarrec jmarrec merged commit d8026ec into develop Nov 2, 2023
4 of 6 checks passed
@jmarrec jmarrec deleted the schedule-file-rel-path branch November 2, 2023 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Request IDDChange Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support relative paths when FTing Schedule:File objects
4 participants