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

Normalized schema additions #524

Merged

Conversation

pablojimpas
Copy link
Contributor

@pablojimpas pablojimpas commented Aug 31, 2022

Summary

References #395

Changes

  • Introduce a new object for soil measurements.
  • Add CO2 and light intensity properties to the air object.

Notes for Reviewers

  • Some definitions are extracted, even though they are simple. I've done this because I think it's a good practice to have quantities and their context decoupled as much as possible, that way quantities can be reused as the schema evolves. This is problematic in some cases, for example, the atmospheric pressure has a range that may not be suitable for industrial valves, in this case, I think it makes more sense to maintain specific definitions inside each context.
  • I've set the maximum of electrical conductivity based on the most conductive metal, silver. In practice, this is probably overkilled.
  • I haven't set a closed range for the pH scale based on this: https://pubs.acs.org/doi/pdf/10.1021/ed083p1465. In practice, maybe it makes more sense to set it to [0, 14] since most sensors on the edge aren't able to measure very high or very low values https://pubs.acs.org/doi/abs/10.1021/ac60120a014. I haven't extracted the pH because it is ridiculously simple with these assumptions.
  • I have only added a small batch of new measurements for now, please let me know if you prefer larger PRs, I leave this as a draft until we agree on the right size.

Release Notes

  • Added support for soil measurements, CO2 and light intensity in the normalized payload schema.

@johanstokking johanstokking self-requested a review August 31, 2022 07:09
@johanstokking johanstokking added this to the 2022 Q3 milestone Aug 31, 2022
@johanstokking johanstokking self-assigned this Aug 31, 2022
Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

Thanks! Few minor comments

I think it's useful to add a depth indicator to soil as well. It would be a value in centimeters.

lib/payload.json Outdated Show resolved Hide resolved
lib/payload.json Outdated Show resolved Hide resolved
lib/payload.json Outdated Show resolved Hide resolved
lib/payload.json Outdated Show resolved Hide resolved
lib/payload.json Outdated Show resolved Hide resolved
johanstokking
johanstokking previously approved these changes Aug 31, 2022
@pablojimpas pablojimpas marked this pull request as ready for review August 31, 2022 23:55
lib/payload.json Outdated Show resolved Hide resolved
@pablojimpas
Copy link
Contributor Author

Sorry, I haven't looked at GitHub in the last few days.
I've fixed that pH description I left off last time.

Other than figuring out the depth and elevation issue, what else is missing for this PR? Is this an acceptable number of additions to the schema, or should we proceed in larger batches?

@cBashTN
Copy link
Contributor

cBashTN commented Oct 3, 2022

Isn't the core idea of the normalizer to have standardized units? If yes, then why use cm instead of m?

@NicolasMrad NicolasMrad modified the milestones: 2022 Q3, 2022 Q4 Oct 10, 2022
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.

5 participants