Support sample_weight in buildstock.csv #1056
Conversation
|
Should the new allowable column, for consistency, be "Sample Weight" instead of "sample_weight"? |
| register_value(runner, 'sample_weight', sample_weight.to_s) | ||
| end | ||
|
|
||
| if !sample_weight.nil? | ||
| register_value(runner, 'weight', sample_weight.to_s) |
There was a problem hiding this comment.
Why are these registered as different variables? Should it just be one or the other gets stored? I do see there is a "weight" and a "sample_weight" in results.json, but then just "sample_weight" is output in results_up00.csv, can you help clarify this?
There was a problem hiding this comment.
I was wondering this, too. But erred on the side of not breaking the current behavior. @nmerket do you know why weight gets registered here? Was this a bsb thing at some point?
There was a problem hiding this comment.
I think it gets registered so that it will show up in the outputs in results.csv. Then we can use it in downstream postprocessing.
There was a problem hiding this comment.
But doesn't it get removed in favor of using sample_weight?
There was a problem hiding this comment.
Any recommendations what should be done here in regard to weight? Nothing? Or should I try removing it and see what happens?
There was a problem hiding this comment.
I'm a little lost on where sample_weight gets used downstream, because it seems like sample_weight never got set here before, only weight
There was a problem hiding this comment.
sample_weight is a measure argument for BuildExistingModel. As long as this argument gets assigned a value, it will "register" as build_existing_model.sample_weight (i.e., you don't need the register_value(runner, 'sample_weight', ...) call). This PR "re-registers" (overwrites) the sample_weight value as it exists in the buildstock.csv.
| workflow_generator: | ||
| type: residential_hpxml | ||
| args: | ||
| build_existing_model: |
There was a problem hiding this comment.
What happens if we also add a sample_weight field here?
There was a problem hiding this comment.
I believe you can already do that. It will override the default calculation here: https://github.com/NREL/buildstockbatch/blob/develop/buildstockbatch/workflow_generator/residential_hpxml.py#L266-L272.
Orrrr maybe the yml schema doesn't permit a sample_weight argument. I'll look into this. You'd have a use case for making sample_weight available here?
There was a problem hiding this comment.
I was wondering what happens if you have a sample_weight here and one in the buildstock, but looking closer at that workflow generator, I guess you can't explicitly set one.
I guess my question still stands though, one is assigned weight and the other sample_weight?
There was a problem hiding this comment.
Currently the yml won't validate if you include sample_weight under the build_existing_model block:
yamale.yamale_error.YamaleError: Error validating data
build_existing_model.sample_weight: Unexpected element
We could update the schema to allow sample_weight. In the case that it's provided, it would just override the n_buildings_represented / n_datapoints calculation. The stuff downstream that this PR addresses would then apply accordingly...
There was a problem hiding this comment.
Do we want to support explicitly setting sample_weight from the yml file?
There was a problem hiding this comment.
No, I don't think that is necessary, bsb still passes sample_weight just not explicitly from the user, so I'm wondering what happens when sample_weight gets passed as an argument and there is a sample_weight column in the buildstock. Seems like the buildstock one overwrites it, right?
There was a problem hiding this comment.
That's correct. The workflow generator will continue to calculate sample_weight. But this PR will allow the presence of a sample_weight column in buildstock.csv to overwrite that calculated value.
Pull Request Description
Adds support for a
sample_weightcolumn in the precomputed buildstock.csv. By default, BuildExistingModel writesbuild_existing_model.sample_weightbased on calculatingn_buildings_represented / n_datapoints(this calculation is done in the workflow generator and then passed into the BuildExistingModel measure). Now, ifsample_weightalready exists in the buildstock.csv, it will write this value instead of the calculated one.This PR also:
weight- it doesn't appear to be used anywherenumber_of_buildings_representedargument from BuildExistingModel - it appears it was only being used in the context of a PAT project, which we no longer supportChecklist
Not all may apply:
Documentation has been updatedopenstudio tasks.rb update_measureshas been run