Add quantity CloudCover #883
Conversation
Codecov Report
@@ Coverage Diff @@
## master #883 +/- ##
==========================================
- Coverage 82.76% 82.72% -0.05%
==========================================
Files 287 291 +4
Lines 43362 43674 +312
==========================================
+ Hits 35888 36128 +240
- Misses 7474 7546 +72
Continue to review full report at Codecov.
|
…rriding of tolerance, added IntegerOktas property.
|
Not sure what the problem is with regards to the AppVeyor build failing. The code compiles for me and all cloud cover unit tests pass. |
It fails on |
|
Ah, it's the windows runtime component code generator. This is a target that is largely deprecated, but kept around for new units via JSON changes. Could you please duplicate the extension method to |
Done. |
|
Bump. |
|
Waiting on #880 to decide on naming. Bumped that thread. |
| <TargetPlatformVersion Condition=" '$(TargetPlatformVersion)' == '' ">10.0.16299.0</TargetPlatformVersion> | ||
| <TargetPlatformMinVersion>10.0.10586.0</TargetPlatformMinVersion> | ||
| <TargetPlatformVersion Condition=" '$(TargetPlatformVersion)' == '' ">10.0.18362.0</TargetPlatformVersion> | ||
| <TargetPlatformMinVersion>10.0.18362.0</TargetPlatformMinVersion> |
There was a problem hiding this comment.
Is there a reason to bump up the MinVersion?
There was a problem hiding this comment.
I think Visual Studio makes this change automatically. But yes, best to revert this.
| /// <param name="min">The minimum value allowed</param> | ||
| /// <param name="max">The maximum value allowed</param> | ||
| /// <returns>The clamped value</returns> | ||
| public static T Clamp<T>(this T val, T min, T max) where T : IComparable<T> |
There was a problem hiding this comment.
This would also work for any IQuantity - we could maybe expose it using a more restrictive (IQuantity) version in UnitMath
|
As discussed in #880, let's go with |
|
Sorry I haven't read up much on the Oktas but doesn't it make more sense to have Clamp as a method on the quantity itself, rather than the internally stored value? Currently it seems that we cannot ensure proper addition/subtraction operations give the lossy nature of the internal round. For example isn't the result from (A + B)/2 likely to be (significantly) different from A/2 +B/2? Also, maybe we should think about flagging the json file somehow, to indicate that it is non-linear (like with the Logarithmic flag) |
@lipchev Good point, but then I propose we rather disable arithmetic codegen? "GenerateArithmetic": false,See Temperature as an example. |
|
@lipchev As for clamping, a better solution might be to add support in codegen for clamping of a quantity - so you could define that CloudCover should be clamped to say [0,1] of base unit - or some selected unit. Then it would inject clamping where appropriately. However, since this is the first quantity that requires it, I'm fine with doing it per unit for now. I think it should do what we want, to not allow less than 0% and not more than 100% cloud cover. |
|
@MarkCiliaVincenti I believe this is the current status of the PR feedback:
|
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Sorry, I saw this almost 2 years late :) What did you mean by renaming to |
|
In JSON, rename SingularName/PluralName Fraction/Fractions to DecimalFraction/DecimalFractions to be consistent with |
Fixes #879