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 #1380

Merged
merged 13 commits into from
Nov 27, 2023
Merged

Schedule:File relative paths #1380

merged 13 commits into from
Nov 27, 2023

Conversation

joseph-robertson
Copy link
Collaborator

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

Pull Request Description

Test NREL/OpenStudio#4879.

Checklist

PR Author: Check these when they're done. Not all may apply. strikethrough and check any that do not apply.

PR Reviewer: Verify each has been completed.

  • Schematron validator (EPvalidator.xml) has been updated
  • Sample files have been added/updated (via tasks.rb)
  • Tests have been added/updated (e.g., HPXMLtoOpenStudio/tests and/or workflow/tests/hpxml_translator_test.rb)
  • Documentation has been updated
  • Changelog has been updated
  • openstudio tasks.rb update_measures has been run
  • No unexpected changes to simulation results of sample files

@joseph-robertson joseph-robertson self-assigned this May 9, 2023
@shorowit
Copy link
Contributor

shorowit commented May 9, 2023

@joseph-robertson Maybe I just don't follow what you're doing, but I wouldn't expect any changes are needed on the OS-HPXML side. OS-HPXML is already creating an OSM that has relative paths in it, they just need to be preserved in the IDF. It shouldn't matter what working directory is being used.

@shorowit
Copy link
Contributor

shorowit commented May 9, 2023

Okay, I partly take that back. OS-HPXML is not currently creating an OSM with a relative path, but doing so just means changing this line to be a relative path. Changing the working directory is risky and should be avoided.

@joseph-robertson
Copy link
Collaborator Author

joseph-robertson commented May 9, 2023

I was trying to change the output_dir from abs to rel (which, correct, I'm now realizing I just need to make the @output_schedules_path relative). But where does the working directory get changed?

On a related note, this gets tricky. We need to pass a valid relative (to current working directory) path into ScheduleFile, but ExternalFile needs to be written with just the basename of that relative path in the "File Name" field. Right? Or can OSM's ExternalFile "File Name" be the original rel path, and it's up to FT to write out just the basename?

@joseph-robertson
Copy link
Collaborator Author

I think we want both the OSM and IDF to have relative paths to the schedule file (alongside the OSM and IDF)?

@joseph-robertson joseph-robertson marked this pull request as ready for review October 27, 2023 23:46
@joseph-robertson
Copy link
Collaborator Author

@shorowit

Not exactly sure how this should be reviewed. Test locally using installer at the bottom of NREL/OpenStudio#4879? Request that a docker image be created so CI can handle it?

@shorowit
Copy link
Contributor

@joseph-robertson Are you trying to get NREL/OpenStudio#4879 into OS 3.7, or is it for the following release?

@joseph-robertson
Copy link
Collaborator Author

@joseph-robertson Are you trying to get NREL/OpenStudio#4879 into OS 3.7, or is it for the following release?

@shorowit Sounds like OpenStudio will have a v3.7.0-rc2. And because NREL/OpenStudio#4879 seems to be working, I think it makes sense to try getting it in this upcoming release.

@shorowit
Copy link
Contributor

Got it. If you're happy with how things are working, then it sounds like you can proceed w/ getting the OS PR merged for rc2. I assume you don't need the CI tests here to pass for that to happen. When rc2 is available, we can update the CI for this PR.

@shorowit shorowit added this to the 1.7.0 milestone Oct 31, 2023
Copy link
Contributor

@shorowit shorowit left a comment

Choose a reason for hiding this comment

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

Looks good. Will merge once the CI comes back green.

@shorowit shorowit merged commit 5b26ce2 into master Nov 27, 2023
6 checks passed
@shorowit shorowit deleted the schedule-file-rel-path branch November 27, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants