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

add customization for revit to openstudio results #134

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

MatthewSteen
Copy link
Member

Adds a small customization to the OpenStudio Results measure for Revit's Systems Analysis workflow from https://github.com/NREL/gbxml-to-openstudio/measures to reduce redundancy and establish a single source of truth. When/if this is merged, the duplicate OpenStudio Results measure can be removed from the gbxml-to-openstudio repo.

@MatthewSteen MatthewSteen marked this pull request as ready for review May 13, 2023 13:27
@DavidGoldwasser
Copy link
Collaborator

DavidGoldwasser commented May 16, 2023

@MatthewSteen I like the idea of having single measure. The new revit argument does two things. Instead of having an argument to set unit it gets it from the OSW, it also adds the E+ eplustbl.

I propose a small modification to make these changes more generally applicable for other use cases.

  1. We already have a units choice argument that has IP and SI as choices. We can add a third option here that says Get Units from OSW File.
  2. the revit argument can be renamed Add EnergyPlus Detailed Report. It looks like the current revit argument is also turning the measure warnings section, but there is already a bool argument for that to be turned off.

@DavidGoldwasser
Copy link
Collaborator

@MatthewSteen I want to add in a test for the new argument option to use units from osw file. Can you proivde a sample OSW file with units. I thought I could generate that from OS App, but it doesn't look like it adds that

@MatthewSteen MatthewSteen marked this pull request as draft June 7, 2023 21:09
@MatthewSteen
Copy link
Member Author

MatthewSteen commented Jun 7, 2023

@DavidGoldwasser I'll work on adding a test for the new units choice, which is an OSRunner method that's not associated with the WorkflowJSON (OSW) class/object. Let me know if I'm missing something.

@DavidGoldwasser
Copy link
Collaborator

DavidGoldwasser commented Jun 8, 2023

@MatthewSteen ahh assumed maybe it was in the OSW and then loaded here
https://github.com/NREL/openstudio-common-measures-gem/blob/update/openstudio-results-for-revit/lib/measures/openstudio_results/tests/OpenStudioResults_Test.rb#L766

Or is the only way to populate the units with another runner method.

@DavidGoldwasser
Copy link
Collaborator

@MatthewSteen I didn't get to this last night, but I can review this afternoon.

@MatthewSteen
Copy link
Member Author

@DavidGoldwasser I'm not ready for another review yet. I need to test the measure with the Revit Systems Analysis workflow before it's ready again. After I've done that I'll change this to ready for review again.

@MatthewSteen
Copy link
Member Author

@DavidGoldwasser one of the customizations is in the report.html.erb file, which replaces the distributed javascript library sources with local copies in the /resources folder.

If this is ok with you, I'll commit those files and add code to replace the sources.

@DavidGoldwasser
Copy link
Collaborator

@MatthewSteen Yes, I'm fine with change to use local javascript files

@MatthewSteen MatthewSteen marked this pull request as ready for review August 5, 2023 00:05
@MatthewSteen
Copy link
Member Author

@DavidGoldwasser this is ready for a review.

@MatthewSteen MatthewSteen marked this pull request as draft January 30, 2024 16:17
@MatthewSteen MatthewSteen marked this pull request as ready for review July 2, 2024 22:53
@MatthewSteen MatthewSteen changed the base branch from develop to 0.10.0-rc1 July 2, 2024 22:55
@MatthewSteen MatthewSteen changed the base branch from 0.10.0-rc1 to develop July 2, 2024 22:55
@MatthewSteen
Copy link
Member Author

MatthewSteen commented Jul 2, 2024

@DavidGoldwasser this is ready for review. Test failure looks unrelated. Please add this to 0.10.0 if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants