Skip to content

Add Turbidity Unit #842

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

Merged
merged 2 commits into from
Sep 27, 2020
Merged

Add Turbidity Unit #842

merged 2 commits into from
Sep 27, 2020

Conversation

rgomez90
Copy link
Contributor

Added Turbidity Unit and conversions

  • NTU
  • mg/l
  • ppm

Fixes #825

@rgomez90
Copy link
Contributor Author

@angularsen I am having issues with the conversions tests.

I suppose I did sth wrong, but I tried to follow the wiki, by giving the conversion values a fixed value.

@rgomez90 rgomez90 changed the title Add Turbidity Unit WIP: Add Turbidity Unit Sep 22, 2020
@lipchev
Copy link
Collaborator

lipchev commented Sep 22, 2020

Hello,
I'm not at all an expert on the subject- but from what I gather- it is not really possible to relate NTU to concentrations. In fact I'm not really sure what concentration you are referring to here- as the Formazin mixture (I'm told) is made up of two different solutions.
Do you have some other standard in mind?

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2020

Codecov Report

Merging #842 into master will decrease coverage by 0.05%.
The diff coverage is 67.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #842      +/-   ##
==========================================
- Coverage   80.54%   80.49%   -0.06%     
==========================================
  Files         281      283       +2     
  Lines       41925    42276     +351     
==========================================
+ Hits        33770    34031     +261     
- Misses       8155     8245      +90     
Impacted Files Coverage Δ
UnitsNet/GeneratedCode/UnitAbbreviationsCache.g.cs 100.00% <ø> (ø)
UnitsNet/GeneratedCode/Quantities/Turbidity.g.cs 67.51% <67.51%> (ø)
UnitsNet/GeneratedCode/Quantity.g.cs 81.43% <80.00%> (-0.02%) ⬇️
...ons/GeneratedCode/NumberToTurbidityExtensions.g.cs 100.00% <100.00%> (ø)
UnitsNet/GeneratedCode/UnitConverter.g.cs 100.00% <100.00%> (ø)
...ratedCode/NumberToMassConcentrationExtensions.g.cs 100.00% <0.00%> (ø)
...et/GeneratedCode/Quantities/MassConcentration.g.cs 87.41% <0.00%> (+1.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce7cc46...e1e0a35. Read the comment docs.

@rgomez90
Copy link
Contributor Author

@angularsen @lipchev I think you're right. I am also not a specialist in this type of unity...But yeah, it seems, the conversion depends on a calibration.

So I suppose, we could add NTU without conversions?

@angularsen
Copy link
Owner

angularsen commented Sep 23, 2020 via email

@rgomez90
Copy link
Contributor Author

@angularsen I've modified the code and removed the conversions to ppm and mg/l.

I think we could merge it already just adding the new NTU unit.

For the conversion, I think we could go the `myTurbidity.ToMiligramsPerLiter(someReference)' way...

They will be just the 2 conversions, to ppm and to mg/l. So manually adding both methods and tests should not be a headache.

@angularsen angularsen changed the title WIP: Add Turbidity Unit Add Turbidity Unit Sep 27, 2020
@angularsen angularsen merged commit 4b078ce into angularsen:master Sep 27, 2020
@angularsen
Copy link
Owner

Ok, merged. Do you plan on adding the conversions later?

@angularsen
Copy link
Owner

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.

Add Unit NTU (Turbidity)
4 participants