Add backporter from 3.8.0 to 3.7.0 - #3
Conversation
I also realized that I should be handling the name changes of some of the variables.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
I've usually gotten the OSMs from osversion/test:
eg this should be the 3.7.0 osm; https://github.com/NatLabRockies/OpenStudio/blob/develop/src/osversion/test/3_8_0/test_vt_HeatExchangerAirToAirSensibleAndLatent.osm
and I'd just use openstudio 3.8.0 to load it and save to 3.8.0
There was a problem hiding this comment.
test_vt_NoLoadSupplyAirFlowRateControlSetToLowSpeed.osm
You don't have any tests for ZoneHVACWaterToAirHeatPump
There was a problem hiding this comment.
from pathlib import Path
import shutil
import sys
OS_OLD_VERSION = "3_7_0"
OS_NEW_VERSION = "3_8_0"
sys.path.insert(0, f"/Applications/OpenStudio-{OS_NEW_VERSION.replace('_', '.')}/Python")
import openstudio
assert OS_NEW_VERSION.replace('_', '.') in openstudio.openStudioLongVersion()
this_dir = Path(f"/Users/julien/Software/Others/OpenStudio/src/osversion/test") / OS_NEW_VERSION
osms = list(this_dir.glob('*.osm'))
TEST_DIR = Path('../tests/').absolute()
assert TEST_DIR.is_dir()
TEST_DIR = Path('../tests/').absolute()
assert TEST_DIR.is_dir()
test_dir = TEST_DIR / OS_NEW_VERSION
test_dir.mkdir(parents=True, exist_ok=True)
for osm in osms:
stem = osm.stem.replace('test_vt_', '')
shutil.copy(osm, test_dir / f"{stem}_{OS_OLD_VERSION}.osm")
m = openstudio.model.Model.load(osm).get()
m.save(test_dir / f"{stem}_{OS_NEW_VERSION}.osm", True)There was a problem hiding this comment.
Ah, I didn't realize that there were already a perfectly good set of test OSMs for me to use here. Thank you for pointing me to the part of the OpenStudio repo and for the sample script here. I have updated the unit tests to use all of these OSMs. So there's now a test for ZoneHVACWaterToAirHeatPump.
I kept a few of the older files that I made for OS:People:Definition and OS:Schedule:Day just because there are so many variations for how these can appear in the OSM schema (depending on whether optional fields have been specified or not).
The extra OS:HeatExchanger:AirToAir:SensibleAndLatent sample OSM I have has been converted to a case to test the curve.evaluate() backporting.
…, 7)] — the sets are iterated in hash order, not insertion order
|
Thanks for the review, @jmarrec . It may not be until Monday but I'll go through all of the suggestions and make some edits. |
|
Thanks again, @jmarrec . This one is now ready for your review. Trying to evaluate the curve for the changes to the AirToAir HeatExchanger proved to be quite the task but you can see from the unit tests that it is working the way that you proposed. This includes both the lookup tables that everyone seems to be using and quadratic curves, which no one is using now as far as I can tell but we know that they could theoretically use them. FYI, they really need to update the sample in the input/output reference here since it's using the old wrong IDD schema for the HeatExchanger. I'll open an issue on the E+ github at some point for this. |
|
Hi @jmarrec , I just wanted to check in and ask if you have a rough time frame on when you might be able to review this PR and merge/release a new version. There is no rush and we can wait for another couple of weeks but we have a few users who have been waiting on us to add OpenStudio backporting capabilities to Ladybug Tools and Pollination (specifically to version 3.7) and so we'll need to make a decision in the near future about whether we want to use the official openstudio-backporter from this repo and PyPI or whether we should just copy our fork of it into our software and distribute it that way. Our preference to give you and this repo all of the credit and PyPI download numbers (which should be a lot given that the Python Software Foundation has contacted us in the past to tell us that the Ladybug Tools packages are among the top 1% of most-downloaded packages on PyPI). But we also don't want to burden you if you think this may create more pressure to update and maintain the package than you were originally hoping for. As you can see with this PR, we are happy to share the maintenance burden and send contributions when we need them but we realize that this can still create a code review burden. If you're not sure at the moment, we can also just start now by distributing our copied fork with Ladybug Tools and then later switch it over to use the official version once you have gotten the chance to review, merge, and release things. So no pressure either way. Just let us know what you're thinking. |
| if iddname == "OS:HeatExchanger:AirToAir:SensibleAndLatent": | ||
|
|
||
| # 4 Fields have been removed from 3.7.0 to 3.8.0: | ||
| # ---------------------------------------------- | ||
| # * Sensible Effectiveness at 75% Heating Air Flow {dimensionless} * 6 | ||
| # * Latent Effectiveness at 75% Heating Air Flow {dimensionless} * 7 | ||
| # * Sensible Effectiveness at 75% Cooling Air Flow {dimensionless} * 10 | ||
| # * Latent Effectiveness at 75% Cooling Air Flow {dimensionless} * 11 | ||
|
|
||
| # 4 Fields have been added from 3.7.0 to 3.8.0: | ||
| # ---------------------------------------------- | ||
| # * Sensible Effectiveness of Heating Air Flow Curve Name * 20 | ||
| # * Latent Effectiveness of Heating Air Flow Curve Name * 21 | ||
| # * Sensible Effectiveness of Cooling Air Flow Curve Name * 22 | ||
| # * Latent Effectiveness of Cooling Air Flow Curve Name * 23 | ||
|
|
||
| # copy the object while inserting fields for the Effectiveness at 75% | ||
| eff_75_indices = (6, 7, 10, 11) | ||
| eff_100_indices = (4, 5, 6, 7) | ||
| eff_curve_indices = (20, 21, 22, 23) | ||
| copy_with_added_fields(obj=obj, newObject=newObject, inserted_indices=set(eff_75_indices)) | ||
|
|
||
| # loop through the effectiveness curves and convert them | ||
| for e100, e75, ec in zip(eff_100_indices, eff_75_indices, eff_curve_indices): | ||
| curve_id = openstudio.toUUID(obj.getField(ec).get()) | ||
| curve_obj = idf_3_8_0.getObject(curve_id) | ||
| if curve_obj: | ||
| curve_obj = curve_obj.get() | ||
| curve_idd_name = curve_obj.iddObject().name() | ||
| if curve_idd_name == "OS:Table:Lookup": # pull the value from the table | ||
| if e75_value := curve_obj.getDouble(11): | ||
| newObject.setDouble(e75, e75_value.get()) | ||
| elif curve_idd_name == "OS:Curve:Quadratic": # reverse translate the curve and evaluate it | ||
| # collect all of the objects the curve references | ||
| temp_model = openstudio.model.Model() | ||
| hx_curve = openstudio.model.CurveQuadratic(temp_model) | ||
| if coeff_value := curve_obj.getDouble(2): | ||
| hx_curve.setCoefficient1Constant(coeff_value.get()) | ||
| if coeff_value := curve_obj.getDouble(3): | ||
| hx_curve.setCoefficient2x(coeff_value.get()) | ||
| if coeff_value := curve_obj.getDouble(4): | ||
| hx_curve.setCoefficient3xPOW2(coeff_value.get()) | ||
| if e100_value := obj.getDouble(e100): | ||
| e100_value = e100_value.get() | ||
| print(hx_curve.evaluate(0.75) * e100_value) | ||
| newObject.setDouble(e75, hx_curve.evaluate(0.75) * e100_value) | ||
|
|
||
| else: # if no curve has been assigned, assume a constant effectiveness | ||
| if value := obj.getString(e100): | ||
| newObject.setString(e75, value.get()) | ||
|
|
||
| targetIdf.addObject(newObject) |
There was a problem hiding this comment.
This isn't defensive enough. It just assumes the table has a given layout.
And it only handles Curve:Quadratic otherwise
There was a problem hiding this comment.
Thanks, @jmarrec ,
I can add an extra check to just use a constant effectiveness whenever the table is not in the two-category 75%/100% format that I see used across the openstudio forward translator, openstuido-standards, and all measures that I have been able to identify using air-to-air heat exchangers. Would this be good enough in your opinion or do we need some logic that tries to handle cases where there are not explicit lookups for the 75% and 100% categories? If you have a sample of another table format that you think we should support, I'll design to that.
What other curves do you think we need to support for this case? It seemed really odd to use a cubic curve for a case but I can see biquadratic curves being used so I'll definitely add those. I can also try to accommodate every possible type of curve. It's just going to be a lot of code given that the curves do not translate from IDF workspace to OSM model objects so I have to explicitly get all of the fields like you see here to make the OSM curve object.
Also, I have no examples that I could find of anyone using any curves for this object. Every case that I have been able to find so far is using the lookup table in the two-category format. So I have to come up with my own curves if we want to have unit tests for them.
There was a problem hiding this comment.
Ah wait, I see you already addressed everything and merged the PR. Thank you, @jmarrec . Your changes are really helpful for me to know for the future.
There was a problem hiding this comment.
FWIW, a biquad curve wouldn't work. It has to have numDimensions() == 1, since it's only function of the airflow (one independent variable).
| OS:Curve:Quadratic, | ||
| {738c67f1-2157-4d0e-a41c-586b64abf8c9}, !- Handle | ||
| Simple DOAS_Heat Recovery Unit Sensible Performance, !- Name | ||
| 1.06, !- Coefficient1 Constant | ||
| 0.191, !- Coefficient2 x | ||
| -0.255, !- Coefficient3 x**2 | ||
| 0, !- Minimum Value of x | ||
| 1; !- Maximum Value of x | ||
|
|
||
| OS:Curve:Quadratic, | ||
| {8a54148f-33ea-42cd-b341-67056a545a23}, !- Handle | ||
| Simple DOAS_Heat Recovery Unit Latent Performance, !- Name | ||
| 1.06, !- Coefficient1 Constant | ||
| 0.191, !- Coefficient2 x | ||
| -0.255, !- Coefficient3 x**2 | ||
| 0, !- Minimum Value of x | ||
| 1; !- Maximum Value of x |
There was a problem hiding this comment.
Where are these coming from out of curiosity? Bogus?
They are the same, and they evaluate to > 1.0 constantly. Which is fine since it's a multiplier curve.
There was a problem hiding this comment.
Where are these coming from out of curiosity? Bogus?
A spreadsheet. Fitting a quadratic curve to the 75%/100% effectiveness values that openstudio standards uses for all of it's enthalpy wheel heat exchangers by default.
I can see why there was probably some E+ developer who wanted to change this field to be a curve for consistency with other types of HVAC objects. But the implementation they did here is a little weird because they ask for the heat recovery effectiveness at 100% and then the curve is supposed to account for the better efficiency that you get when running the heat exchanger close to it's sweet spot at 75%. So you are pretty much forced to use curves that return values above 1 to have realistic heat exchanger behavior. Probably, they should have asked for peak effectiveness rather than effectiveness at 100%.
In any event, thanks again for taking care of all of the curve translations, @jmarrec.
| def _evaluate_regular_curve(curve_obj: openstudio.IdfObject, x_values: list[float]) -> list[float]: | ||
| """Evaluate a curve object at a given x value. | ||
|
|
||
| Args: | ||
| ----- | ||
| * curve_obj: (openstudio.IdfObject) The curve object to evaluate | ||
| * x_values: (list[float]) The x values at which to evaluate the curve | ||
|
|
||
| Returns: | ||
| ------- | ||
| * (list[float]) The evaluated y values of the curve at the given x values | ||
|
|
||
| Preconditions: | ||
| -------------- | ||
| * curve_obj must be a valid curve object (not OS:Table:Lookup) | ||
| * It assumes the Curve object did not get any IDD changes between 3.8.0 and the version of openstudio you use | ||
| """ | ||
|
|
||
| curve_idd = curve_obj.iddObject() | ||
| curve_idd_name = curve_idd.name() | ||
| if curve_idd_name == "OS:Table:Lookup": | ||
| raise ValueError("OS:Table:Lookup curves cannot be evaluated directly") | ||
|
|
||
| m = openstudio.model.Model() | ||
|
|
||
| model_curve_idd = m.iddFile().getObject(curve_idd_name) | ||
| assert model_curve_idd.is_initialized(), f"Curve type '{curve_idd_name}' not found in IddFile" | ||
| model_curve_idd = model_curve_idd.get() | ||
|
|
||
| # We check that the IDD matches the one in the model, to ensure that the curve object is compatible with the model | ||
| n_this = curve_idd.numFields() + curve_idd.properties().numExtensible | ||
| n_model = model_curve_idd.numFields() + model_curve_idd.properties().numExtensible | ||
| if n_this != n_model: | ||
| raise ValueError( | ||
| f"Curve object of type '{curve_idd_name}' has {curve_idd.numFields()} fields, " | ||
| f"but the model expects {model_curve_idd.numFields()} fields. " | ||
| "This indicates that the curve object is not compatible with the model." | ||
| ) | ||
| for i in range(n_this): | ||
| this_field = curve_idd.getField(i).get() | ||
| model_field = model_curve_idd.getField(i).get() | ||
| if this_field.name() != model_field.name(): | ||
| raise ValueError( | ||
| f"Curve object of type '{curve_idd_name}' has field '{this_field.name()}' at index {i}, " | ||
| f"but the model expects field '{model_field.name()}'. " | ||
| "This indicates that the curve object is not compatible with the model." | ||
| ) | ||
|
|
||
| o_ = m.addObject(openstudio.IdfObject(model_curve_idd)) | ||
| assert o_.is_initialized(), f"Failed to add curve object of type '{curve_idd_name}' to model" | ||
| curve = o_.get().to_Curve().get() | ||
|
|
||
| if curve.numVariables() != 1: | ||
| raise ValueError( | ||
| f"Curve object of type '{curve_idd_name}' has {curve.numVariables()} variables, but only 1 is supported" | ||
| ) | ||
|
|
||
| for i in range(curve_obj.numFields()): | ||
| if value := curve_obj.getString(i): | ||
| curve.setString(i, value.get()) | ||
|
|
||
| return [curve.evaluate(x) for x in x_values] |
There was a problem hiding this comment.
Handle all curve types, not just Curve:Quadratic
| def _evaluate_table_lookup( | ||
| idf_3_8_0: openstudio.IdfFile, curve_obj: openstudio.IdfObject, x_values: list[float] | ||
| ) -> list[float] | None: | ||
| if curve_obj.iddObject().name() != "OS:Table:Lookup": | ||
| raise ValueError("Only OS:Table:Lookup curves are accepted") | ||
|
|
||
| if curve_obj.getString(3).get().lower() != 'divisoronly': | ||
| logger.warning(f"{brief_description(idf_obj=curve_obj)} is not 'DivisorOnly', cannot evaluate.") | ||
| return None | ||
| divisor = curve_obj.getDouble(4).value_or(1.0) | ||
| assert divisor > 0, f"{brief_description(idf_obj=curve_obj)}: Divisor must be greater than 0, got {divisor}" | ||
|
|
||
| ind_var_list_uid = openstudio.toUUID(curve_obj.getString(2).get()) | ||
| ind_var_list_ = idf_3_8_0.getObject(ind_var_list_uid) | ||
| if not ind_var_list_.is_initialized(): | ||
| raise ValueError(f"{brief_description(idf_obj=curve_obj)}: independent variable list is not found.") | ||
| ind_var_list = ind_var_list_.get() | ||
| if ind_var_list.numExtensibleGroups() != 1: | ||
| raise ValueError( | ||
| f"{brief_description(idf_obj=curve_obj)}: independent variable list has " | ||
| f"{ind_var_list.numExtensibleGroups()} extensible groups, expected 1." | ||
| ) | ||
| ind_var_uid = openstudio.toUUID(ind_var_list.getExtensibleGroup(0).getString(0).get()) | ||
| ind_var_ = idf_3_8_0.getObject(ind_var_uid) | ||
| if not ind_var_.is_initialized(): | ||
| raise ValueError(f"{brief_description(idf_obj=curve_obj)}: independent variable is not found.") | ||
| ind_var = ind_var_.get() | ||
|
|
||
| interp_method = ind_var.getString(2).get().lower() | ||
| extrap_method = ind_var.getString(3).get().lower() | ||
| if interp_method != 'linear': | ||
| logger.warning(f"{brief_description(idf_obj=ind_var)}: not 'Linear' for interpolation, " "cannot evaluate.") | ||
| return None | ||
| if extrap_method != 'linear': | ||
| logger.warning(f"{brief_description(idf_obj=ind_var)}: not 'Linear' for extrapolation, " "cannot evaluate.") | ||
| return None | ||
|
|
||
| y_values: list[float | None] = [None for _ in x_values] | ||
|
|
||
| if curve_obj.numExtensibleGroups() != ind_var.numExtensibleGroups(): | ||
| raise ValueError( | ||
| f"{brief_description(idf_obj=curve_obj)}: the number of extensible groups in the curve and " | ||
| "independent variable do not match." | ||
| ) | ||
|
|
||
| xs = [] | ||
| ys = [] | ||
| for i, (x_eg, y_eg) in enumerate(zip(ind_var.extensibleGroups(), curve_obj.extensibleGroups())): | ||
| x = x_eg.getDouble(0).get() | ||
| y = y_eg.getDouble(0).get() | ||
| xs.append(x) | ||
| ys.append(y / divisor) | ||
|
|
||
| for i, x in enumerate(x_values): | ||
| if x < xs[0] or x > xs[-1]: | ||
| logger.warning( | ||
| f"{brief_description(idf_obj=curve_obj)}: the x value {x} is outside the range of the independent " | ||
| f"variable ({xs[0]} to {xs[-1]}), cannot evaluate." | ||
| ) | ||
| continue | ||
|
|
||
| for j in range(1, len(xs)): | ||
| if x <= xs[j]: | ||
| # Linear interpolation | ||
| y = ys[j - 1] + (ys[j] - ys[j - 1]) * (x - xs[j - 1]) / (xs[j] - xs[j - 1]) | ||
| y_values[i] = y | ||
| break | ||
|
|
||
| # Verify that all x values were evaluated | ||
| for i, y in enumerate(y_values): | ||
| if y is None: | ||
| logger.warning(f"{brief_description(idf_obj=curve_obj)}: the x value {x_values[i]} could not be evaluated.") | ||
| return None | ||
|
|
||
| return y_values # type: ignore[return-value] |
There was a problem hiding this comment.
Defensive programming for the Table Lookup
| for e100, e75, ec in zip(eff_100_indices, eff_75_indices, eff_curve_indices): | ||
| curve_uid = openstudio.toUUID(obj.getField(ec).get()) | ||
| curve_obj_ = idf_3_8_0.getObject(curve_uid) | ||
| if curve_obj_: | ||
| curve_obj = curve_obj_.get() | ||
| curve_idd_name = curve_obj.iddObject().name() | ||
| x_values = [0.75, 1.0] | ||
|
|
||
| if curve_idd_name == "OS:Table:Lookup": | ||
| y_values = _evaluate_table_lookup(idf_3_8_0=idf_3_8_0, curve_obj=curve_obj, x_values=x_values) | ||
| if y_values is None: | ||
| logger.warning( | ||
| f"{brief_description(idf_obj=obj)}: Effectiveness curve '{curve_obj.name().get()}' " | ||
| "is a table lookup that cannot be evaluated, skipping conversion and using " | ||
| "constant effectiveness instead." | ||
| ) | ||
| y_values = [1.0, 1.0] | ||
| else: | ||
| y_values = _evaluate_regular_curve(curve_obj=curve_obj, x_values=x_values) | ||
|
|
||
| e100_value = obj.getDouble(e100).value_or(1.0) | ||
| y75, y100 = y_values | ||
|
|
||
| newObject.setDouble(e75, y75 * e100_value) | ||
| # If y100 isn't near 1.0, we warn | ||
| if abs(y100 - 1.0) > 1e-3: | ||
| logger.warning( | ||
| f"{brief_description(idf_obj=obj)}: Effectiveness curve '{curve_obj.name().get()}' " | ||
| f"evaluated at 100% flow is {y100:.6f}, expected 1.0. " | ||
| "This may indicate that the curve is not normalized to 1.0 at 100% flow." | ||
| ) |
There was a problem hiding this comment.
evalaute the curve, and also warn if the 100% value isn't 1.0
|
@chriswmackey I've made changes, which took me a few hours I didn't have, but I'm doing this as a courtesy to you and respect for your and ladybug's contribution to the community. I'll make a release. |
3 participants
This PR adds a backporter from version 3.8.0 to 3.7.0 following the changes from the OpenStudio ForwardTrasnlator here in the source code. Thanks, @jmarrec, for pointing us towards this in #2.
I added unit tests for all objects that changed, and in the process, realized that I should probably be handling the name change of the accpetable field values in the backporter.
As far as I can tell, this is also the first case we have of fields that existed in the old OpenStudio version, which were removed in the newer version (specifically with
OS:HeatExchanger:AirToAir:SensibleAndLatent). So I added a newcopy_with_added_fieldsfunction to the helpers to assist with these cases.Let me know if you have any review comments and I am happy to address them, @jmarrec . Once we have backporting to 3.7.0 supported, we should be able to integrate this into Ladybug Tools to help people get older OSMs when they need them.
Resolves #2