-
Notifications
You must be signed in to change notification settings - Fork 10
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
Rhorsey/sampling v2 plotting patch #217
Conversation
|
||
def add_weights_aportioned_by_stock_estimate(self, apportionment: Apportion, keep_n_per_apportionment_group=False): | ||
# This function doesn't support already CBECS-weighted self.data - error out | ||
if self.CBECS_WEIGHTS_APPLIED: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GREAT!
@@ -2478,7 +2548,8 @@ def rmv_units(c): | |||
assert new.startswith(old) | |||
logger.debug(f'{old} -> {new}') | |||
|
|||
self.data = self.data.rename(crnms) | |||
lazyframe = lazyframe.rename(crnms) | |||
return lazyframe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return lazyframe.rename(crnms)
will be exctaly same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I tried to run comstock_to_cbecs_comparison.py with the default template but not works due to eulp/euss_com/cycle_4_sampling_test_rand_985932_20240321/cycle_4_sampling_test_rand_985932_20240321/
is not existed, the right path should be cycle_4_sampling_test_rand_985932_20240321/rand_985932_20240321/baseline/
Thanks @wenyikuang - updated the paths and also switched to using Resource instead of Client in boto3 to avoid an error with the policy on stuff uploaded from Kestrel using the default upload policy. |
@ChristopherCaradonna can you delete the lighting upgrade in you comstock_data and try rerunning compare_upgrades again? If that works as expected can you review / approve please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rHorsey Postprocessing completed, most plots look good.
2 Issues:
- Census Division plots show no energy / square footage in "Middle Atlantic".
- Non-breaking error message, as show below
Bonus) I get an error for not having 'import json' in the s3_utilties_mixin.py
--- Logging error ---
Traceback (most recent call last):
File "C:\Users\ccaradon\AppData\Local\anaconda3\envs\comstockpostproc\lib\logging_init_.py", line 1083, in emit
msg = self.format(record)
File "C:\Users\ccaradon\AppData\Local\anaconda3\envs\comstockpostproc\lib\logging_init_.py", line 927, in format
return fmt.format(record)
File "C:\Users\ccaradon\AppData\Local\anaconda3\envs\comstockpostproc\lib\logging_init_.py", line 663, in format
record.message = record.getMessage()
File "C:\Users\ccaradon\AppData\Local\anaconda3\envs\comstockpostproc\lib\logging_init_.py", line 367, in getMessage
msg = msg % self.args
TypeError: not all arguments converted during string formatting
Call stack:
File "C:\Users\ccaradon\Documents\GitHub\ComStock\postprocessing\compare_upgrades.py", line 73, in
main()
File "C:\Users\ccaradon\Documents\GitHub\ComStock\postprocessing\compare_upgrades.py", line 58, in main
comstock.add_national_scaling_weights(cbecs, remove_non_comstock_bldg_types_from_cbecs=True)
File "C:\Users\ccaradon\Documents\GitHub\ComStock\postprocessing\comstockpostproc\comstock.py", line 1900, in add_national_scaling_weights
logger.info("sf wt_area_col shape: ", sf[wt_area_col].shape)
Message: 'sf wt_area_col shape: '
Arguments: ((14,),)
# comstock.create_geospatially_resolved_aggregations(comstock.STATE_ID, pretty_geo_col_name='state_id') | ||
# comstock.create_geospatially_resolved_aggregations(comstock.COUNTY_ID, pretty_geo_col_name='county_id') | ||
# TODO Long is def not working as expected anymore... | ||
# comstock.export_to_csv_long() # Long format useful for stacking end uses and fuels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is ComStock wide export?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the create_national_aggregations method now - same code under the hood but redone to get correctly weighted national level results!
@ChristopherCaradonna 's issues addressed - ready for approval! Great catch on mid-atlantic - many thanks!. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, approved.
Pull request overview
Updates plotting scripts for comparing upgrades, baselines, and CBECS. AMI and Timeseries are up next - EIA requires a transition to our current postproc paradigm.
Pull Request Author
This pull request makes changes to (select all the apply):
Author pull request checklist:
Added tests for new measuresUpdated measure .xml(s)Register values added tocomstock_column_definitions.csv
Bothoptions_lookup.tsv
files updatedChange documentation writtenMeasure documentation writtenComStock documentation updatedChanges reflected in example.yml
filesChanges reflected inREADME.md
filesImplements corresponding measure tests and indexing path intest/measure_tests.txt
or/andtest/resource_measure_tests.txt
All new and existing tests pass the CIReview Checklist
This will not be exhaustively relevant to every PR.
ComStock Licensing Language - Add to Beginning of Each Code File