Boavizta / boaviztapi

🛠 Giving access to BOAVIZTA reference data and methodologies trough a RESTful API
GNU Affero General Public License v3.0
66 stars 23 forks source link

#214 add rounding based on min and max #220

Closed PierreRust closed 8 months ago

PierreRust commented 9 months ago

This function should be used when the API returns a value with an associated min and max value. The rounding depends on the delta betwwen min and max

da-ekchajzer commented 9 months ago

@PierreRust I have made an implementation with a max_sig_fig in the config file which corresponds to the uncertainty of the impact factors. Please give you feedback before I adapt the tests.

PierreRust commented 9 months ago

@da-ekchajzer looks fine to me, from what I see you apply the sig_fig after the min-max based rounding, which is what I was thinking of.

Once this is done we could open a new issue on the calculation on the uncertainty, based on a uncertainty on the reference number.

da-ekchajzer commented 9 months ago

While adapting the test, I came up with an actual case where min and max are too wide, and we come with a value of zero.

It is the impact of a SSD where the density is not given.

curl -X 'GET' \
  'http://dev.api.boavizta.org/v1/component/ssd?verbose=true&archetype=DEFAULT&criteria=gwp&criteria=adp&criteria=pe' \
  -H 'accept: application/json'
{'adp': {'description': 'Use of minerals and fossil ressources',
         'embedded': {'max': 3.151,
                      'min': 0.006863,
                      'value': 0.0,
                      'warnings': ['End of life is not included in the '
                                   'calculation']},
         'unit': 'kgSbeq',
         'use': 'not implemented'},
 'gwp': {'description': 'Total climate change',
         'embedded': {'max': 110000.0,
                      'min': 226.3,
                      'value': 0.0,
                      'warnings': ['End of life is not included in the '
                                   'calculation']},
         'unit': 'kgCO2eq',
         'use': 'not implemented'},
 'pe': {'description': 'Consumption of primary energy',
        'embedded': {'max': 1365000.0,
                     'min': 2807.0,
                     'value': 0.0,
                     'warnings': ['End of life is not included in the '
                                  'calculation']},
        'unit': 'MJ',
        'use': 'not implemented'}}

I believe that this is inconsistent with the LCA principle of never putting zero to a value. I understand that such a large uncertainty may justify setting zero, but at the same time, I have the feeling that returning the min/max values makes it possible to be transparent about the uncertainty.

To solve this type of case, I would have suggested having at least 1 significant figure and adding a warning to mention a large uncertainty margin.

I have made an implementation. You can check the tests to see the behavior of this update.

PierreRust commented 9 months ago

I agree, I think we should add a safeguard to make sure the value is never lower than the min, maybe by rounding up instead of rounding down. I'll see how we could implement that.