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

construction_set_u_value wrongful call of find_and_set_insulaton_layer #1423

Closed
omarh119 opened this issue Feb 24, 2023 · 10 comments
Closed
Assignees
Milestone

Comments

@omarh119
Copy link

insulation_layer_name = find_and_set_insulaton_layer(construction).name

This probably should be construction.find_and_set_insulation_layer.name.

Current usage returns the following error

NoMethodError (undefined method 'find_and_set_insulaton_layer' for #<ASHRAE9012007:0x00007fc5ffc9c198>)
Did you mean? find_and_set_insulation_layer

@mdahlhausen
Copy link
Collaborator

Thanks for finding this.
Looks like a simple misspelling error.
It should be find_and_set_insulation_layer not find_and_set_insulaton_layer

insulation_layer_name = self.find_and_set_insulaton_layer(construction).name

find_and_set_insulation_layer is defined later on in the file:

def find_and_set_insulation_layer(construction)

It looks like the BTAP portion of the code is still using the misspelled method:

def self.find_and_set_insulaton_layer()

And the BTAP method adds it as a method on top of the Construction object, rather than taking a construction argument

@phylroy it looks like you originally wrote this. Can you correct the typo, harmonize the methods, and remove the separate BTAP version?

@phylroy
Copy link
Collaborator

phylroy commented Feb 26, 2023

Can do.. should I add it to our nrcan branch and you'll get it on our next merge?

@mdahlhausen
Copy link
Collaborator

@phylroy either way is fine

@phylroy
Copy link
Collaborator

phylroy commented Feb 27, 2023

okay I'll chat with chris on when we plan to have nrcan branch ready.

@phylroy
Copy link
Collaborator

phylroy commented Mar 9, 2023

So it turns out that this method is not really used by anything, along with a few others. It think this was leftover from the migration in Dec 2017 that Andrew and I did. I am nervous about deleting the code, so I am commenting it out for now..but will remove it in the next version if all is well.
testing now on branch nrcan_376 and will pull into nrcan if successful tomorrow.

@omarh119
Copy link
Author

omarh119 commented Mar 9, 2023

This wouldn't affect the Standard.find_and_set_insulation_layer(construction) method, right? If you can kindly confirm this will help as I'm currently using this in some scripts. Thanks.

@phylroy
Copy link
Collaborator

phylroy commented Mar 9, 2023

omarh119. It should not since you are using the class Standard. This method was in the OpenStudio::Model::Construction module namespace. So unless your work used that.. it should not be impacted.

@phylroy
Copy link
Collaborator

phylroy commented Mar 10, 2023

The refactor has been merged in nrcan_os351_merge as commit revision a794506

This commit also removes confusing tests that are not run anymore in nrcan branch.. these have been removed in the Rakefile as well. All relevant tests passed. Waiting on @ckirney to merge with NREL's code.

@phylroy phylroy closed this as completed Mar 10, 2023
@mdahlhausen
Copy link
Collaborator

Re-opening the issue because the spelling error find_and_set_insulation_layer vs. find_and_set_insulaton_layer remains.

@mdahlhausen mdahlhausen reopened this Mar 11, 2023
@mdahlhausen
Copy link
Collaborator

Fixed with #1435

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

No branches or pull requests

4 participants