NREL / OpenStudio

OpenStudio is a cross-platform collection of software tools to support whole building energy modeling using EnergyPlus and advanced daylight analysis using Radiance.
https://www.openstudio.net/
Other
509 stars 193 forks source link

API allows you to enter invalid object names #1597

Closed DavidGoldwasser closed 8 years ago

DavidGoldwasser commented 9 years ago

I ran acros this testing fix for issue #1591. It seems like the fix is more likely related to changes made between 1.7.1 and 1.7.2 than fixes made for BCL attributes. Below are screenshots of the issue. It doesn't seem to matter what weather file I chose. I hit the same thing. If you look at screenshots from #1591 you can see that it didn't hit the issue then?

Screenshots in apply measures now 4-13-2015 9-45-12 am

4-13-2015 9-45-35 am

4-13-2015 10-25-56 am

Similar issue in PAT. It happens in model to IDF, but I assume I would hit it sooner if we had more OSM measures. 4-13-2015 10-19-37 am

Here is the text I see when I try to open the OSM made by the measures ruby measure test. `` Error: Model with Version 1.7.2 IDD is not valid to draft strictness level.

Error: The collection is INVALID at strictness level 'Draft', because of the errors: Field level data error of type DataType . Error is in an object of type 'OS:Site', named 'Denver Centennial Golden Nr', in field 2. Additional information about the error type: field-level data is of an incorrect type. Field level data error of type NumericBound . Error is in an object of type 'OS:Site', named 'Denver Centennial Golden Nr', in field 4. Additional information about the error type: numeric data violates a min or max bound. ``

DavidGoldwasser commented 9 years ago

Here is the site object from the ruby test. OS:Site, {34fceb3c-6e0d-46e1-8ddf-19844eea695a}, !- Handle Denver Centennial Golden Nr, CO, USA, !- Name 39.74, !- Latitude {deg} -105.18, !- Longitude {deg} -7, !- Time Zone {hr} 1829, !- Elevation {m} ; !- Terrain

Here is the site object for model when epw is added via GUI. OS:Site, {d47e4844-0925-42b2-85e9-db469d25fa1a}, !- Handle Site 1, !- Name 39.74, !- Latitude {deg} -105.18, !- Longitude {deg} -7, !- Time Zone {hr} 1829, !- Elevation {m} ; !- Terrain

Looks like I had bad mixing and matching of code in the. I added the site section from change building location, but it didn't use commas in weather name, while the plugin code did. That should fix this issue, so I'll lower severity, but API should still be better.

# Add or update weather data
weather_name = "#{epw_file.city}, #{epw_file.stateProvinceRegion}, #{epw_file.country}"
weather_lat = epw_file.latitude
weather_lon = epw_file.longitude
weather_time = epw_file.timeZone
weather_elev = epw_file.elevation

# Add or update site data
 site = model.getSite
 site.setName(weather_name)
 site.setLatitude(weather_lat)
 site.setLongitude(weather_lon)
 site.setTimeZone(weather_time)
 site.setElevation(weather_elev)

@macumber shouldn't the API not allow you to set a name for an object that it knows will make it fail to open in the future?

DavidGoldwasser commented 9 years ago

I updated title reflect the cause of the issue. People shoudl be able to ignore this if they know the issue, but seems like the API should be smart enough to avoid it.

eringold commented 8 years ago

Ran into a similar problem when a comma is included in the Name field of a User-Defined Measure associated with a PAT alternative run. The Name string is set as-is as the name of the Life Cycle Cost EnergyPlus object, fouling up the idf fields.

DavidGoldwasser commented 8 years ago

@macumber if I run the code below it does stop me, saying it expected a string.

schedule.setName(fan)

That makes sense, but if I instead type one of the lines of code below, it lets me set this complex multi-line string as an object name. It will open the OSM after this which it didn't in the past.

schedule.setName(fan.to_s) or schedule.setName("#{fan}_sch")

In the example what the measure writer intended to do was.

schedule.setName("#{fan.name}_sch")

The result is no longer a corrupted file, but I expect may still cause downstream issues with either forward translation or E+, and certainly causes GUI issues viewing the schedule name.

Do we want to allow long multi-line strings? if we are ok with that, then we can close this now.

macumber commented 8 years ago

@DavidGoldwasser can you verify if this still applies?

DavidGoldwasser commented 8 years ago

It still lets you use a multi-line-string as an object name. Not sure if that is an issue. OS app opens fine and writes to IDF, not sure if this is good idea. We could close or leave open as minor edge low frequency?

Here is an example where construction object is used as name for building object.

Building, Construction&#44&#10 {1c0f4459-ab3e-4a05-b673-a3b7605e1a44}&#44 &#33- Handle&#10 ASHRAE 189.1-2009 ExtRoof IEAD ClimateZone 1&#44 &#33- Name&#10 &#44 &#33- Surface Rendering Name&#10 {f82ec3d4-faa7-411a-8ea3-bc49515fac98}&#44 &#33- Layer 1&#10 {cca1140e-e997-46e0-ab39-add5ea04377f}&#44 &#33- Layer 2&#10 {e78b997d-5e86-4641-beda-ef585c18aef0}&#59 &#33- Layer 3&#10&#10, !- Name , !- North Axis {deg} , !- Terrain , !- Loads Convergence Tolerance Value , !- Temperature Convergence Tolerance Value {deltaC} , !- Solar Distribution , !- Maximum Number of Warmup Days ; !- Minimum Number of Warmup Days

macumber commented 8 years ago

Well it looks ugly but to me it is ok, does it show up badly in the App? I'm not sure if the line edits for those fields support multi line strings

DavidGoldwasser commented 8 years ago

It does look ugly in the app, but really this should only happen with badly written measure renaming object. I'll close it.