Skip to content
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

change meter reading from real to float #651

Merged
merged 1 commit into from
Sep 9, 2021
Merged

change meter reading from real to float #651

merged 1 commit into from
Sep 9, 2021

Conversation

huss
Copy link
Member

@huss huss commented Aug 28, 2021

Description

The meter table added for v0.7.0 stores the last seen reading uploaded to OED . This value was a real. It is being changed from a real (32-bit) to a float (64-bit) for several reasons:

  1. Most other places in OED involving readings use float.
  2. It was much more likely that OED would truncate the value for cumulative data since this is the one place where the raw meter value is stored and cumulative data gets larger value. The next reading would then subtract from this one so the final error could be much larger. For example, it rounded to 6 digits (decimal) to store in DB and when it subtracted suppose the first 4 digits are the same as the next reading so the 6th digit is the 2nd in the final answer. This means the error could now be 5 parts in 100 or 5%. Any large impact is exceedingly unlikely with 64-bit/15 digit accuracy.
  3. Unfortunate differences could occur depending on how whether values were in a single CSV file or two that split at the value in question for cumulative data. The JS code should be using 64-bit arithmetic as it goes through readings so it acts in the way OED would if it was float in the DB. However, with the current real in the DB, if the last value of a CSV is stored on the meter and rounded to 32-bit it leads to what is described above. This means the next value input will have different arithmetic vs. if all values were in a single CSV upload. Changing to float eliminates this.
  4. It isn't common to access the reading on the meter and it isn't in time critical code. Thus, any slowdown is not significant. Also, the space used in the DB is trivial given the number of meters.

Type of change

  • Note merging this changes the node modules
  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request

Limitations

Note that no migration is being provided since this is all within the v0.7.0 release. A note will be posted to developers about what to do if they are inflight.

While this fix addresses the issue on meter readings, it does not address the parallel issue that reading values are real and not float. That is more complex but at least does not involve raw/cumulative values being stored. Issue #650 addresses that.

Want 8 byte rather than 4 byte values. PR will explain.
@huss huss requested a review from lindavin August 28, 2021 20:50
@huss huss added this to the 0.7 release milestone Aug 28, 2021
@huss huss mentioned this pull request Aug 30, 2021
5 tasks
@huss huss closed this Aug 30, 2021
@huss huss reopened this Sep 2, 2021
Copy link
Contributor

@lindavin lindavin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think looks good! Thanks for the work!

@huss huss merged commit 61aa95c into development Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants