-
Notifications
You must be signed in to change notification settings - Fork 56
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
Appendix g dev #1612
Appendix g dev #1612
Conversation
…s not assigned, which cause the incorrect baseline construction assignment, and ultimately energyplus simulation failure.
…missing_construction_name Wx bug fix fenestration expansion missing construction name
…andards into wx/baseline_oa
…cation Add assertions for getting data from additional properties Add new file import in openstudio-standards.rb Add new process in the master function under Standards.Model.rb
Wx/baseline oa
update the prm_raise
…ations Complete run four orientations
…baseline_window_to_wall_ratio
…_baseline_window_to_wall_ratio Appx g/enhancement/model apply prm baseline window to wall ratio
# @param data_key [string] The data key to retrieve the data from the OpenStudio object | ||
# @param remaining_keys [str] Any additional keys in the path | ||
# @return the OpenStudio Object or exception raise | ||
def prm_get_optional_handler(component, log_dir, data_key, *remaining_keys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this function necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have many situations to get optional object from an OpenStudio object.
spacetype = space.spaceType.get.standardsSpaceType.get
In above code, if a space has no spaceType
or standardsSpaceType
, then ruby would raise exception with message nil has no get function
. This happens quite often with models generated from BEM software. The error message is not informative.
The prm_get_optional_handler
is designed to handle such scenario to give more informative error messages to user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The better practice is to call .is_initialized each time, and then have comments at that point in code if the optional value is not initialized.
Search .is_initialized in the codebase and you'll see many examples of if - else statements to catch this sort of thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but there are chained optionals in the PRM codebase, so this method comes in handy to handle those chained optional get.
I can add .is_initialized in the prm_get_optional_handler
method to reflect the best practice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see 20 uses of this method in the code base, and 1 instance of chaining, which I elsewhere commented wasn't necessary.
I think this method doesn't accomplish what you want - giving more clear error messages to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the PRM routine, there are growing number of chaining for processing user data including some of them pointing to name
.
Examples include:
Get building user data name
get space types:
Most of them are specific to PRM user data processing so I would like, if possible, to keep it for now. is_initialized
can handle if a specific optional data exist or not, but the detail implementation to handle this condition varies. This method is convenient to handle is_initialized = false
condition to fail the process with information on which optional object is missing from a component.
I can update the error message to include an OpenStudio.logFree(OpenStudio::Error, 'openstudio.standards.utilities', 'key xxx is missing from xxx object...) before line 47 so we get a user message saying something
is missing, instead of nil has no get function
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the use of global variables instead of instance variables for class UserData?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about using global variables, but it loses some functionalities specific to a group of data.
For example, there are many flags in the user data that require user to type true
or false
in the CSV file.
To check whether user provided correct string to these flags, we need to have a way to verify those strings are not yes
or no
and they are case insensitive.
The advantage of class implementation is that the UserData class can have a generic matched_any?
method which compares a string with a list of expected enums.
In the case described above, we created a UserDataBoolean, a subclass of UserData, and specified expected enums (true
and false
) as instance variables to run the matched_any?
.
lib/openstudio-standards/standards/ashrae_90_1_prm/ashrae_90_1_prm.Surface.rb
Show resolved
Hide resolved
lib/openstudio-standards/standards/ashrae_90_1_prm/ashrae_90_1_prm.SpaceType.rb
Outdated
Show resolved
Hide resolved
lib/openstudio-standards/standards/ashrae_90_1_prm/ashrae_90_1_prm.SpaceType.rb
Outdated
Show resolved
Hide resolved
lib/openstudio-standards/standards/ashrae_90_1_prm/ashrae_90_1_prm.SpaceType.rb
Outdated
Show resolved
Hide resolved
lib/openstudio-standards/standards/ashrae_90_1_prm/ashrae_90_1_prm.SpaceType.rb
Outdated
Show resolved
Hide resolved
lib/openstudio-standards/standards/ashrae_90_1_prm/ashrae_90_1_prm.SpaceType.rb
Outdated
Show resolved
Hide resolved
lib/openstudio-standards/standards/ashrae_90_1_prm/ashrae_90_1_prm.SpaceType.rb
Outdated
Show resolved
Hide resolved
lib/openstudio-standards/standards/ashrae_90_1_prm/ashrae_90_1_prm.SpaceType.rb
Outdated
Show resolved
Hide resolved
lib/openstudio-standards/standards/ashrae_90_1_prm/ashrae_90_1_prm.SpaceType.rb
Outdated
Show resolved
Hide resolved
end | ||
|
||
# Get pipe node | ||
node = prm_get_optional_handler(pipe, @sizing_run_dir, 'to_StraightComponent', 'outletModelObject', 'to_Node') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use the outletModelObject field from the StraightComponent parent class instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That line is equivalent to pipe.to_StraightComponent.outletModelObject.to_Node
, so we do use the outletModelObject
field from the StraightComponent
parent class. Could you please clarify what you would like to see?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just replace it with node = pipe.to_StraightComponent.outletModelObject.to_Node
also not sure node_name is used in the following lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't to_Node
return an optional? prm_get_optional_handler
handles optionals. I can remove the node name.
test/90_1_prm/test_appendix_g_prm.rb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have mentioned this last time:
Split out this file into a helper methods file, and a test file. The test_appendix_g_prm should only include the test_ methods. (It sometimes makes sense to break this practice, but I don't it makes sense here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make senses. Given the time constraint, could we move this to next release? I will open an issue in our backlog to address it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's simple:
- move non-test methods to another file
- add a require statement at the top of the test file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, removed.
It could be further improved, I will put those work to our blacklog. Thanks @mdahlhausen
# | ||
# @param [OpenStudio::Model::ModelObject] | ||
# @return Casted OpenStudio object or nil if the cast was not possible | ||
def model_cast_model_object(model_object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method is unnecessary and could result in weird behavior, particularly for HVAC where it is called. I suggest refactoring to remove it in the two places it is used. Happy to think about how to refactor those methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is actually quite convenient. For example, with a few lines of code, it allows us to check if an HVAC related component in a model is autosized without actually knowing what its full object type name is in advance. Without it, we would have to list in the code all the objects that we want to check. The proposed approach also supports future objects to be included in OpenStudio/EnergyPlus (i.e., new coil objects). It has existed in the codebase for a while now, we just generalized it here to use it in multiple places.
What are the weird behavior that you'd expect to see? If you don't want to expose it we could make it private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can probably delete this file as methods are unused or could be refactored and embedded inline.
@@ -1,4 +1,5 @@ | |||
require 'csv' | |||
require 'date' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this in the core or std library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comes from Ruby standard library - see Ruby 2.7.2
lib/openstudio-standards/standards/ashrae_90_1_prm/ashrae_90_1_prm.Model.rb
Show resolved
Hide resolved
@@ -2182,6 +2272,7 @@ def model_get_bat_wwr_target(bat, wwr_list) | |||
# @param total_fene_m2 [Float] total fenestration area | |||
# @return [Float] reduction factor | |||
def model_get_wwr_reduction_ratio(multiplier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method should be surface_get_wwr_reduction_ratio.
And then just pass in the surface. You pass in name, and results from methods with surface as an argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you suggest passing in a surface and then get surface_name
, surface_wwr
and surface_dr
shall be a local variable inside the model_get_wwr_reduction_ratio
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function has updated to surface_get_wwr_reduction_ratio
with surface
as a new argument to replace surface_name
, surface_wwr
and surface_dr
…_helper 2. Modify the function model_get_wwr_reduction_ratio to surface_get_wwr_reduction and reduce its argument to surface. - Update YARD document
Pull request overview
Merge Appendix G PRM development.
Features:
Enhancements:
run_four_orientation
using methods in exception handler module.update_ground_temperature_profile
using methods in exception handler module.model_identify_non_mech_cooled_systems
using methods in exception handler module.apply_baseline_exterior_lighting
using methods in exception handler module, move user data to user data process functionmodel_add_prm_elevators
using methods in exception handler module.Bug fixs:
Pull Request Author
bundle exec rake doc
)bundle exec rake rubocop
)require
statements, ensure these are in core ruby or add to the gemspecReview Checklist
This will not be exhaustively relevant to every PR.