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

fix/97 - simulation issues #98

Merged
merged 71 commits into from
Dec 16, 2020
Merged

fix/97 - simulation issues #98

merged 71 commits into from
Dec 16, 2020

Conversation

corymosiman12
Copy link
Contributor

Initial work started for: #97

  • cleaned up redundant parameters passing, method(element, ns) -> method by initializing superclasses for LocationElement, SpatialElement, etc.
  • removed awkward weather dir lookup
  • strict requirements on single facility

@corymosiman12 corymosiman12 marked this pull request as draft November 13, 2020 07:06
@corymosiman12 corymosiman12 marked this pull request as ready for review December 16, 2020 01:45
- $SH "bundle exec rspec spec/tests/model_articulation_test/service_hot_water_system_spec.rb"
- $SH "bundle exec rspec spec/tests/model_articulation_test/site_spec.rb"
# - $SH "bundle exec rspec spec/tests/model_articulation_test/weather_file_download_spec.rb" // fails sometimes due to connection issues, so we exclude it here
name: "Model Articulation Tests"
- script:
- travis_wait 40 $SH "bundle exec rspec spec/tests/model_articulation_test/hospital_occupancy_type_spec.rb"
Copy link
Member

Choose a reason for hiding this comment

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

We are going to run out of travis credits pretty soon. Leave this as-is, but we will need to move over to github actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are already out...

Copy link
Member

Choose a reason for hiding this comment

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

travis definitely fits in 2020

@@ -47,4 +49,29 @@ require 'openstudio/model_articulation'
rake_task = OpenStudio::Extension::RakeTask.new
rake_task.set_extension_class(OpenStudio::ModelArticulation::Extension)

desc 'Convert tabs to spaces'
task :remove_tabs do
Copy link
Member

Choose a reason for hiding this comment

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

yes!

# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
# *******************************************************************************

SCHEMA_2_0_URL = 'https://raw.githubusercontent.com/BuildingSync/schema/v2.0/BuildingSync.xsd'
Copy link
Member

Choose a reason for hiding this comment

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

thanks for making these obvious. Is there documentation (in the README) saying to update these when upgrading the version of BuidngSync?

return xml_element.text
end
return nil
end

def help_get_text_value_as_float(xml_element)
Copy link
Member

Choose a reason for hiding this comment

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

nice helpers!

Copy link
Member

@nllong nllong left a comment

Choose a reason for hiding this comment

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

holy cow you did a bunch of work on this! Good job!

I started going through the details. In general your coding looks good and you have followed good conventions. I am good with this if the tests pass.

Again, awesome job getting this all working!

@corymosiman12
Copy link
Contributor Author

Still some things outstanding:

Tests failing:

model_articulation_test/*

  • 8 total
  • 4 tests from facility_spec.rb. Minor fixes needed.
  • 2 tests from weather_file_download_spec.rb - this fails sometimes due to connection issues. noted in .travis.yml
  • 1 for SmallHotel - see previous
  • 1 from site_spec.rb - OSM generated not same as previous...need to look into.

@corymosiman12 corymosiman12 merged commit fe0673e into DA_Update Dec 16, 2020
@corymosiman12 corymosiman12 deleted the fix/97 branch December 16, 2020 17:24
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