Issue604 kpis disaggregated #606
Conversation
If called multiple times, xxxx_dict_by_source used to increase even when the test case is not moving forward because it was calculated from xxxx_dict instead of using the _get_data_from_last_index method.
…tot. Notice that this does not affect to the xxxx_tot results.
|
@JavierArroyoBastida Thanks for this. I'm not sure I will get to this today but will try to sometime next week. |
There was a problem hiding this comment.
@JavierArroyoBastida Thanks for this and your patience. Generally this is looking good. A few inline comments.
One thing I'm thinking is that I've learned a bit in REST API design since original implementation of BOPTEST's, and I think a better way to do this would be create an endpoint GET /kpi/disaggregated. But, no other APIs use this address extension format (yet). So, I'm inclined to maybe just keep what you propose for now and this might change with a more comprehensive API format adjustment down the line (i.e. when consolidating into only using the Service architecture). Open to any discussion around this though.
Some additonal to-dos I'm thinking of:
- update user guide with new api endpoint
- it might be good to add a section in the design guide describing this disaggregation (e.g. ensuring to also include clarification on what the values mean for peak power, whether it is the peak power of the individual element considered separately or it is the contribution to the total peak at that time).
- resolve any conflicts
| @@ -1,5 +1,5 @@ | |||
| keys,value | |||
| PCoo_y,0.0 | |||
| PFan_y,0.000108999039431 | |||
| PHea_y,0.111074370884 | |||
| PFan_y,0.00523195389267 | |||
There was a problem hiding this comment.
Are these changing because of a correction on accounting for area or not within the disaggregated dictionary values? Same with pgas_dict_MultiZone.csv.
There was a problem hiding this comment.
Exactly, this is due to 08a4dfc. For some reason for peak KPIs we were normalizing with the area at the contributions of each element already which is reflected at the pele_dict and pgas_dict dictionaries. This was not consistent with the rest of KPIs where we normalize only when calculating the total value (xxxx_tot).
| '''Return the core KPIs of a test case disaggregated and | ||
| with absolute values (not normalized by area or zone) | ||
| to see the contributions of each element to each KPI. | ||
| Parameters |
There was a problem hiding this comment.
Add space before Parameters.
| Price scenario for cost kpi calculation. | ||
| 'Constant' or 'Dynamic' or 'HighlyDynamic'. | ||
| Default is 'Constant'. | ||
| Returns |
| dkpi = dict | ||
| Dictionary with the core KPIs disaggregated and | ||
| with absolute values. | ||
| ''' |
README.md
Outdated
| @@ -76,6 +76,7 @@ Example RESTful interaction: | |||
| | Receive control signal point names (u) and metadata. | GET ``inputs`` | | |||
| | Receive test result data for the given point names between the start and final time in seconds. | PUT ``results`` with required arguments ``point_names=<list of strings>``, ``start_time=<value>``, ``final_time=<value>``| | |||
| | Receive test KPIs. | GET ``kpi`` | | |||
| | Receive test KPIs disaggregated and in absolute values. | GET ``kpi_disaggregated`` | | |||
There was a problem hiding this comment.
@JavierArroyoBastida Suggest a bit more clarity: "Receive test KPIs disaggregated into contributing components (e.g. each equipment or zone) ... "
|
Thanks for the review @dhblum! I'll go through your comments by next week. |
…_kpisDisaggregated # Conflicts: # testing/references/bestest_hydronic/submit.json # testing/references/testcase3/submit.json
Changed from 'pgas_tot': 0.12017492256526957 to 'pgas_tot': 0.12017492256526958
…esidential_hydronic.
# Conflicts: # releasenotes.md
…_kpisDisaggregated
|
@dhblum I finally got tests to pass and I believe this is ready for a second review. The change introduced in |
Closes #604. @dhblum could you take a look?