search

IEA-Task-43 / digital_wra_data_standard

IEA Task 43: pre-construction energy estimate data standard repository
BSD 3-Clause "New" or "Revised" License
58 stars 16 forks source link

Fixing data types from last pull request for the calibration units. #267

Closed kersting closed 3 weeks ago

kersting commented 3 months ago

EDIT: This is for Issue #248

kersting commented 2 months ago

@abohara thanks for the review.

@stephenholleran I'll wait for you to be back before merging in case you have additional plans for other pull requests.

stephenholleran commented 2 months ago

I have fixed up everything to match the original pull request (with the type fixes).

The next thing to do is to make these new slope_units, offset_units and sensitivity_units to be an enum which means the measurement_units needs to be made into a definition. I see problems with this but let me make the changes so you can see.

stephenholleran commented 2 months ago

@kersting @abohara

Not as bad as I thought it would be though there are some inconsistencies. To make it work I didn't include the Title or Definition in the new measurement_units definition. This was to allow slope_unit to have it's own Definition.

This definitely needs a good review from both of you.

Thanks,

stephenholleran commented 2 months ago

@kersting @abohara

From our call today I have made the updates to the units in the SQL statements. It is now ready for both your reviews.

Thanks,

kersting commented 2 months ago

@stephenholleran I reviewed the changes, and everything seems good to me. Do you want me to merge the pull request?

stephenholleran commented 1 month ago

Thanks @kersting.

It would be great for @abohara to take a look first? @abohara the main thing here is moving the 'measurement_units' to be a definition so they could be used by the new 'slope_unit', 'offset_unit' and 'sensitivity_unit' variables.

Thanks,

abohara commented 1 month ago

@kersting @stephenholleran Looks good to me. The only thing I am wondering why "1" and "-" are units.

stephenholleran commented 1 month ago

Thanks @abohara.

The "-" represents dimensionless units. When something doesn't have units we don't want to put null as that could mean the user doesn't know. They can use the - to indicate that they do know and that it has no units. This has been in the data model for a long time, not introduced in this PR.

I had to go looking for the "1". It comes from the digital calibration certificate. I copied all the units in the digital calibration certificate into this and this "1" was there already. I don't actually know what it means myself either. @heikowestermann could you tell us what the "1" represents?

If we do have a problem with the "1", I think that would be a separate issue for us to take it out.

stephenholleran commented 3 weeks ago

@abohara are you ok with this? Thanks,

abohara commented 3 weeks ago

Thanks @stephenholleran . Looks good.