search

SEED-platform / seed

Standard Energy Efficiency Data (SEED) Platform™ is a web-based application that helps organizations easily manage data on the energy performance of large groups of buildings.
Other
107 stars 55 forks source link

Unit-aware columns had display names appended with display units #2327

Closed adrian-lara closed 3 years ago

adrian-lara commented 4 years ago

TL;DR

Should we bring back column display names being shown with pints_units appended?

Background

There's evidence that in previous versions of SEED (maybe as far back as 2017), pints-unit-aware columns had their display names appended with (<pints unit>) for the "list" column endpoint.

Note, I haven't seen any issues or heard any conversations around this being a desired feature.

Research - Technical details

https://github.com/SEED-platform/seed/blob/9e23e67342cf33c156dfe7b8b4983c4b4919b022/seed/serializers/pint.py#L81-L105

The column "list" endpoint uses this method, but provides it with a dictionary of column info with snake_case keys whereas the method expects camelCase keys.

Some fix options

I'd suggest we remove the use of add_pint_unit_suffix in the "list" endpoints as it's, at best, misleading, at worst, an easy entry point for bugs if later changes occur.

I suggest this happens with the introduction of API v3, where a new query param is introduced in order to enable or disable the append logic. This would be needed since the unit should only be added as a suffix in read-only cases.

This would include the inventory list page, inventory list profile page, and likely others. I would suggest that the column admin page would not be included since the display names can actually be changed here, and the "last-minute" addition of the suffix would add unnecessary complications.

A caveat here would be that the confirm mapping page does not yet show the incoming data with appropriate pint conversions. We could either avoid append suffixes to columns, or change it so the incoming data shows the conversion at this point in the import process. For example, when importing Occupied Floor Area as square feet but displaying as meters squared, the confirm mapping page shows the areas as the original square feet value, without conversion, while the column would read "Occupied Floor Area (m[^2])".

Instance Information

Server Instance: prod, dev1, any recent version of SEED

adrian-lara commented 4 years ago

@nllong @axelstudios @RDmitchell Any historical context or other thoughts on previously being able to see display units for applicable columns - Conditioned Floor Area (m²), Site EUI (kBtu/a/ft²), etc.

RDmitchell commented 4 years ago

@adrian-lara -- I was actually thinking about this when I was testing the units aware fields. It's hard to tell which are which, ie units non aware and units aware, when the units are not added to the field.

Of course, some of the ESPM fields have the units as part of their field name, which is generally what users map to from what I have seen, so in that case it wouldn't be possible to distinguish the ESPM field that includes the units from the units aware field that has the units added.

But I do think it would be good to add the units to the fields when the program is keeping track of the units.

adrian-lara commented 4 years ago

After chatting with the group, it seems that this feature has been not working for a very long time, so we can treat and prioritize this as a "new" feature. In my mind, this is a nice-to-have, so I'll probably flag this as priority 3 given we haven't heard anything from users as this being something they want. Of course, anyone feel free to override me there.

Regarding this,

Of course, some of the ESPM fields have the units as part of their field name, which is generally what users map to from what I have seen, so in that case it wouldn't be possible to distinguish the ESPM field that includes the units from the units aware field that has the units added.

I'm not sure if I have any great ideas as to how to avoid or address this problem. I think it might be better to make this change first, then use any user feedback to iterate from there.

RDmitchell commented 4 years ago

@adrian-lara -- I agree with all your points in the comment above. Thanks!

RDmitchell commented 3 years ago

I spent about a half hour (10/2/2020) trying to figure out why San Jose's Site EUI value wasn't the same as the value in the original ESPM spreadsheet. It finally occurred to me to look at the units settings for the org, and they were set to kWh/m2/year, rather than kBtu/sf/year. But I couldn't tell that from looking at the data in the Inventory List or Detail view. Also couldn't see it in the mapping or mapping review screens (after the fact).

We really need to display the units for the fields -- add the units to the column headers, etc.

Steps to Reproduce

change the units for site eui and the value changes image image

Instance Information

Instance: seedv2 SHA: 2e7a2a0d

Ryoken commented 3 years ago

https://github.com/SEED-platform/seed/pull/2453

RDmitchell commented 3 years ago

image

image

Looks good !!!

dreneden1 commented 3 years ago

Units are not showing up in the inventory columns for me, although this would be super helpful. Based on the above, sounds like this feature had been added back, right? I'm I doing something wrong for them to not show up?

adrian-lara commented 3 years ago

@Ryoken can you look into this?

Ryoken commented 3 years ago

@dreneden1 I am going to try and find out why units are not showing up for you. Could I grab some quick info from you so I know which account, organization and properties you are looking at?