Skip to content

Conversation

@brgix
Copy link
Member

@brgix brgix commented Aug 5, 2025

Related to this pyOSut PR.

Around 40-odd typos (and other oddities) were identified in this Ruby implementation of OSut, while developing a Python counterpart, pyOSut. In some cases, these are simple fixes (no impact on the Python implementation). In other cases (like this first commit), they reveal weaknesses (to fix), such as not adequately catching bad user input. Over the next day or so, a number of such fixes are expected to be introduced, before re-releasing an upcoming OSut v0.6.1.

@brgix brgix self-assigned this Aug 5, 2025
@brgix brgix added the enhancement New feature or request label Aug 5, 2025
a[:compo ][:id ] = "OSut|#{mt}|#{format('%03d', d*1000)[-3..-1]}"
when :window
a[:glazing][:u ] = specs[:uo ]
a[:glazing][:u ] = u ? u : @@uo[:window]
Copy link
Member Author

Choose a reason for hiding this comment

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

A user could set a glazing assembly's specs["uo"] to nil. This is caught and reset here.

if u and unglazed
ro = 1 / u - film

if ro > 0
Copy link
Member Author

Choose a reason for hiding this comment

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

For opaque construction, adjusting insulating layer thickness (to meet a requested assembly Uo factor) is skipped entirely if the requested Uo isn't:

  • Numeric
  • within postulated (acceptable) range

expect(surface).to be_a(OpenStudio::Model::LayeredConstruction)
expect(surface.layers.size).to eq(1)
u = 1 / cls1::rsi(surface)
expect(u).to be_within(TOL).of(uo[:skylight])
Copy link
Member Author

Choose a reason for hiding this comment

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

In the case of fenestrated assemblies, an invalid Uo request is ignored and the glazing assembly inherits a default OSut Uo (here 3.5 W/m2.K for a skylight).

m1 = "#{id}:spandrel"
m2 = "#{id}:spandrel:boolean"
return mismatch(id, s, cl, mth) unless s.is_a?(cl)
return mismatch(id, s, cl, mth, false) unless s.is_a?(cl)
Copy link
Member Author

Choose a reason for hiding this comment

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

Yikes.


id = s.nameString
return mismatch(id, s, cl, mth, false) unless s.is_a?(cl)
return mismatch(id, s, cl, mth, DBG, false) unless s.is_a?(cl)
Copy link
Member Author

Choose a reason for hiding this comment

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

Also yikes.


values.each do |value|
if value.respond_to?(:to_f)
vals << value.to_f
Copy link
Member Author

@brgix brgix Aug 6, 2025

Choose a reason for hiding this comment

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

Validate time series data for scheduled temperatures: Log + skip if invalid, then move on.

res[:spt] = max unless res[:spt]
res[:spt] = max if res[:spt] < max
end
end
Copy link
Member Author

Choose a reason for hiding this comment

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

Safe (?) to now check for ScheduleInterval classes.

# @return [nil] if invalid input (see logs)
def trueNormal(s = nil, r = 0)
mth = "TBD::#{__callee__}"
mth = "OSut::#{__callee__}"
Copy link
Member Author

Choose a reason for hiding this comment

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

Remnant.


# Once joined, re-adjust Z-axis coordinates.
unless z.zero?
unless z.round(2) == 0.00
Copy link
Member Author

Choose a reason for hiding this comment

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

z is a float - not an integer.

start = model.getYearDescription.makeDate(1, 1)
inter = OpenStudio::Time.new(0, 1, 0, 0)
values = OpenStudio.createVector(Array.new(8760, 22.78))
series = OpenStudio::TimeSeries.new(start, inter, values, "")
Copy link
Member Author

Choose a reason for hiding this comment

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

OSut enabled ScheduleFixedInterval checks, e.g.:

  • min/max setpoint temperatures
  • is space UNCONDITIONED, etc.

Yet no unit test had been in place.

return pts if pts.size < 3
return mismatch("n collinears", n, Integer, mth, DBG, v) unless ok
return invalid("+n collinears", mth, 0, ERR, v) if n > pts.size
return invalid("-n collinears", mth, 0, ERR, v) if n < 0 && n.abs > pts.size
Copy link
Member Author

Choose a reason for hiding this comment

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

Checks range of n number of points - harmonized with pyOSut.

a2 = flatten(a2).to_a if flat
cw2 = clockwise?(a2)
a2 = a2.reverse if cw2
return invalid("points 2", mth, 2, DBG, face) unless xyz?(a2, :z)
Copy link
Member Author

Choose a reason for hiding this comment

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

Eliminates redundancies between aligned vs non-aligned cases, harmonized with pyOSut.

return face if p1.empty?
return face if p2.empty?
return mismatch("ray", ray, cl, mth) unless ray.is_a?(cl)
return mismatch("ray", ray, cl, mth, face) unless ray.is_a?(cl)
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes (wrong) return value.

# @param [Set<OpenStudio::Point3d>] a triad (3D points)
#
# @return [Set<OpenStudio::Point3D>] a rectangular ULC box (see logs if empty)
# @return [Set<OpenStudio::Point3D>] a rectangular BLC box (see logs if empty)
Copy link
Member Author

@brgix brgix Aug 9, 2025

Choose a reason for hiding this comment

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

Fixes (incorrect) vertex sequencing (documentation issue).

next if same?(p1, sg.last)
next if same?(p2, sg.first)
next if same?(p2, sg.first)
next if same?(p2, sg.last)
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes redundant check - brings in correct check.

id = "plate # #{i+1} (index #{i})"

return mismatch(id, plt, cl1, mth, DBG, slb) unless plt.is_a?(cl2)
return mismatch(id, plt, cl2, mth, DBG, slb) unless plt.is_a?(cl2)
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes incorrect class check. No unit test had been written to stress-test this, so never got caught.

elsif fpm2.keys.include?("strips")
pattern = "strips"
else fpm2.keys.include?("strip")
else # fpm2.keys.include?("strip")
Copy link
Member Author

Choose a reason for hiding this comment

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

Ruby ignores anything following an else, so the hash key check is ignored. Keeping as comment.

# Original polygon remains unaltered.
# vtx2 = mod1.poly(vtx)
# expect(mod1.same?(vtx, vtx2)).to be true
# expect(mod1.status).to eq(0)
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed updating.

slab = mod1.genSlab(plates, z0)
expect(mod1.debug?).to be true
expect(mod1.logs.size).to eq(1)
expect(mod1.logs.first[:message]).to include("String? expecting Hash")
Copy link
Member Author

Choose a reason for hiding this comment

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

New stress-test when requested floor plates (input) aren't hashes.

return pts
elsif pts.is_a?(OpenStudio::Model::PlanarSurface)
pts.vertices.each { |pt| v << OpenStudio::Point3d.new(pt.x, pt.y, pt.z) }
return v
Copy link
Member Author

Choose a reason for hiding this comment

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

This would otherwise return an Array.

return v if pts.empty?
return mismatch("n unique points", n, Integer, mth, DBG, v) unless ok

if n.is_a?(Numeric)
Copy link
Member Author

Choose a reason for hiding this comment

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

Safer to check for Numeric than respond_to?(:to_i). Non-numeric strings respond_to?(:to_i), for instance.

a = sg.first
b = sg.last
a = sg[ 0]
b = sg[-1]
Copy link
Member Author

Choose a reason for hiding this comment

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

Safer. An admissible input segment could be an OpenStudio::Point3dVector (which would crash with a call to .last.

end

if holds?(a, pts[0])
if a.include?(pts[0])
Copy link
Member Author

Choose a reason for hiding this comment

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

Quicker, same object ID.

sg0 = sg.to_a
sgX = sX[j].to_a
sg0 = sg
sgX = sX[j]
Copy link
Member Author

Choose a reason for hiding this comment

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

As each returned item from an OpenStudio::Point3dVectorVector is an Array in Ruby (and not an OpenStudio::Point3dVector), there would be no point in further converting an Array to an Array.


# Stress test 'to_p3Dv'. 4 valid input cases.
# Valid case #1: a single Point3d.
vtx = mod1.to_p3Dv(p0)
Copy link
Member Author

Choose a reason for hiding this comment

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

All 4 valid to_p3Dv use cases systematically return an OpenStudio::Point3dVector (and not occasionally an Array).

# @return [false] if invalid input (see logs)
def segment?(pts = nil)
pts = to_p3Dv(pts)
return false if pts.empty?
Copy link
Member Author

Choose a reason for hiding this comment

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

Redundant.

return empty("segments", mth, DBG, false) if sgs.empty?
return mismatch("point", p0, cl, mth, DBG, false) unless p0.is_a?(cl1)
return empty("segments", mth, DBG, false) if sgs.empty?
return mismatch("point", p0, cl1, mth, DBG, false) unless p0.is_a?(cl1)
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops - no cl variable. Clearly untested.

str2 = str1 + " #{tag.to_s}"
next if st.key?(:void) && st[:void]
return mismatch(str1, st, Hash, mth, DBG, a) unless st.respond_to?(:key?)
next if st.key?(:void) && st[:void]
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. Better to test for a Hash before attempting to access a key:value pair.

return mismatch(str, ld, Hash, mth, DBG, a) unless ld.is_a?(Hash)
return hashkey( str, ld, s, mth, DBG, a) unless ld.key?(s)
return mismatch(str, ld[s], cl, mth, DBG, a) unless ld[s].is_a?(cl)
return mismatch(str2, ld, Hash, mth, DBG, a) unless ld.is_a?(Hash)
Copy link
Member Author

Choose a reason for hiding this comment

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

No str variable!

# previously-added leader lines.
#
# @todo: revise approach for attics ONCE skylight wells have been added.
olap = nil
Copy link
Member Author

Choose a reason for hiding this comment

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

Redundant.

w = opts[:size].to_f
w2 = w * w
return invalid(size, mth, 0, ERR, []) if w.round(2) < gap4
return invalid("size", mth, 0, ERR, []) if w.round(2) < gap4
Copy link
Member Author

Choose a reason for hiding this comment

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

No variable size - clearly a typo.

spaces = spaces.reject { |sp| sp.floorArea < 4 * w2 }
spaces = spaces.sort_by(&:floorArea).reverse
return empty("spaces", mth, WRN, 0) if spaces.empty?
return empty("spaces", mth, WRN, []) if spaces.empty?
Copy link
Member Author

Choose a reason for hiding this comment

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

Wrong return variable.

frame = nil if f.frameWidth.round(2) < 0
frame = nil if f.frameWidth.round(2) > gap
frame = nil if frame.frameWidth.round(2) < 0
frame = nil if frame.frameWidth.round(2) > gap
Copy link
Member Author

Choose a reason for hiding this comment

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

Typo: clearly untested.

when :sloped then filters.map! { |f| f.include?("c") ? f.delete("c") : f }
when :plenum then filters.map! { |f| f.include?("d") ? f.delete("d") : f }
when :attic then filters.map! { |f| f.include?("e") ? f.delete("e") : f }
when :sidelit then filters.map! { |fl| fl.include?("b") ? fl.delete("b") : fl }
Copy link
Member Author

Choose a reason for hiding this comment

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

f already used (designates frame width): use fl instead.

expect(mod1.same?(collinears[1], p1)).to be true

# Ignore n request when n.abs > number of actual collinears.
collinears = mod1.getCollinears([p0, p1, p2, p3, p8], 6)
Copy link
Member Author

Choose a reason for hiding this comment

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

Harmonized tests with pyOSut.

#
# @return [OpenStudio::Point3dVector] unique points (see logs)
def getUniques(pts = nil, n = 0)
def uniques(pts = nil, n = 0)
Copy link
Member Author

Choose a reason for hiding this comment

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

As per pyOSut convention, getter functions drop the get prefix.

# function can be time consuming for very convoluted spaces (e.g. long
# corridors with multiple concavities).
expect(mod1.spaceHeight(fine)).to be_within(TOL).of(8.53)
expect(mod1.spaceWidth(fine)).to be_within(TOL).of(21.33)
Copy link
Member Author

@brgix brgix Aug 11, 2025

Choose a reason for hiding this comment

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

warhouse

The value returned by spaceHeight (8.53 m) corresponds to the tallest floor-to-ceiling distance (along Z-axis).

The floor section circumscribed (and crisscrossed) with red dotted lines corresponds to the largest bounded box that fits in the (large) L-shaped floor surface. The length of the narrowest edge of this bounded box (21.33 m) is what spaceWidth returns.

res = realignedFace(polyg.to_a.reverse)
return 0 if res[:box].nil?

height(res[:box])
Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to add the key piece (bounding box from realignFace).

expect(mod1.alignedWidth(floor)).to be_within(TOL).of(8.22)
expect(mod1.spaceHeight(openarea)).to be_within(TOL).of(3.96)
expect(mod1.spaceWidth(openarea)).to be_within(TOL).of(7.90)
expect(mod1.spaceWidth(openarea)).to be_within(TOL).of(3.77)
Copy link
Member Author

@brgix brgix Aug 12, 2025

Choose a reason for hiding this comment

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

dim
  • in red: full height of space, including skylight wells
  • in green: width of bounding box of space floor(s), outline is very approximate
  • in blue: width of largest bounded box that can fit within grouped floor surfaces

The latter is retained here as the most adequate measure of what could be considered a room's width, e.g. for LPD adjustment calculations.

@brgix brgix merged commit 4d49032 into develop Aug 14, 2025
7 checks passed
@brgix brgix deleted the v061 branch August 14, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants