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

Mobile Coverage points calculator #824

Merged
merged 115 commits into from
Jun 12, 2024

Conversation

michaeldjeffrey
Copy link
Contributor

@michaeldjeffrey michaeldjeffrey commented Jun 6, 2024

Many changes to the rewards algorithm are contained in and across many HIPs. The blog post MOBILE Proof of Coverage contains a more thorough explanation of many of them. It is not exhaustive, but a great place to start.

Please comment if...

  • You think something can be named more appropriately
  • You have tests you'd like to see
  • You think something is important and we should try to make it less hidden
  • You can provide additional context to some part of how scores are calculated
  • Saying hi
  • You think more explanation could be provided to someone perusing the code
  • You feel like

Example Usage

type RadioId = (PublicKeyBinary, Some(String));

pub async fn coverage_points(
    &mut self,
    radio_id: &RadioId,
) -> anyhow::Result<coverage_point_calculator::CoveragePoints> {
    let pubkey = &radio_id.0;
    let average = self.speedtest_averages.get_average(pubkey).unwrap();
    let speedtests = self.speedtests.get(&radio_id).expect("speedtests").clone();
    let radio_type = self.radio_type(&radio_id);
    let trust_scores = self
        .trust_scores
        .get(&radio_id)
        .expect("trust scores")
        .clone();
    let verified = self.radio_threshold_verified(&radio_id);
    let ranked_coverage = self.coverage_map.hexes(&radio_id);

    let coverage_points = coverage_point_calculator::CoveragePoints::new(
        radio_type,
        RadioThreshold::Verified,
        speedtests,
        location_trust_scores,
        ranked_coverage,
    )?;

    Ok(coverage_points)
}

Fields:

Notable Conditions:

  • Location
    • If a Radio covers any boosted hexes, LocationTrust scores must meet distance requirements, or be degraded.
    • CBRS Radio's location is always trusted because of GPS.
  • Speedtests
    • The latest 6 speedtests will be used.
    • There must be more than 2 speedtests.
  • Covered Hexes
    • If a Radio is not BoostedHexStatus::Eligible, boost values are removed before calculations.

Copy link
Contributor

@andymck andymck left a comment

Choose a reason for hiding this comment

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

Still digesting....

coverage_point_calculator/src/location.rs Outdated Show resolved Hide resolved
coverage_point_calculator/src/lib.rs Outdated Show resolved Hide resolved
coverage_point_calculator/src/lib.rs Outdated Show resolved Hide resolved
coverage_point_calculator/src/lib.rs Show resolved Hide resolved
moved to using decimals now that summing lists is involved, and we need to retain accuracy
still need to decide how the multipliers are going to be indexed. 0-indexed is easy, but it seems what to talk about a radio being "0th ranked" in a hex.
the speedtest multiplier is allowed to be zero. It can completely negate a radios rewards.
speedtest 2
it seems a more accurate name for where we are currently, I think
For the way we talk about Ranking of radios, it doesn't make much sense for a radio to be ranked 0th for a hex.
Removing hexes that do not meet the rank requirements is easier to think about in its own step
the HIP says estimated coverage points, but no one thinks of them that way, because there is nothing about them that is an esimate, they are very concrete values.
@bbalser
Copy link
Contributor

bbalser commented Jun 7, 2024

My understanding is the caller will call make_rewardable_radio and then take the output and call calculate_coverage_points. Looking through the code, some of the calculations are done in the first function and some in the second. I don't understand the value of this being 2 steps. Why not just a single function that does everything?

This puts the information about the radio closer to each other, the
grouping feels nicer, I think
this is stepping towards having a single struct
This means we don't need to unwrap anything, and the API is more
strictly dealing with data alone.

The location funtions are only public to the crate to prevent someone
from attempting to put together they're own coverage points calculator.

They must go through the calculate_coverage_points function.
Same as location, we want to deal more directly with data, while keeping
the only way to use the crate be through the top level
calculate_coverage_points function.
dealing more directly with data.
forcing use through the top level api.
I thought `coverage_point_calculator::calculate_coverage_points()`
didn't read very well.

And since we now have a single struct to care about (outside of
providing arguments), it seemed to me
`coverage_point_calculator::CoveragePoints::new()` read rather nicely.
I don't think a zeroed out Speedtest is a sensible default
@michaeldjeffrey
Copy link
Contributor Author

Why not just a single function that does everything?

Having the Radio and CoveragePoints separate was an artifact of the way I started. Now that we're closing in on the end, I agree it makes sense to provide a single function as the interface for calculating coverage points.

In doing so, it also revealed that many of the intermediary structs were no longer needed.
They have been removed in lieu of functions that I think better describe the problem.

@michaeldjeffrey michaeldjeffrey marked this pull request as ready for review June 7, 2024 22:02
Copy link
Contributor

@maplant maplant left a comment

Choose a reason for hiding this comment

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

Much improved

I had typed the wrong value when bringing over megabytes per second conversion. We found some values in a real database during testing that should have gotten a different speedtest tier.
For consistency, the calculator is now truncating values that would have been truncated be the user of the calculator, so we avoid a situation where 2 users of the calculator are using different rounding strategies. 

The fields that make up the truncated values are provided untouched.

Truncating fully to a u64 causes calculations to be off by quite a margin.
@michaeldjeffrey michaeldjeffrey force-pushed the mj/mobile-coverage-points-calculator branch from 63ddf10 to 1abeb84 Compare June 11, 2024 00:34
To keep consistent with the reward calculations this crate is replacing, any rounding of values needs to be avoided before rewards are calculated. This means users of the values will need to handle routing going into protobufs themselves.
When a hex is boosted by a provider, it's oracle (assignment) boosting multiplier is automatically pushed to 1x.
hip-103: top level oracle/provider boosting test
Come up with names for different scenarios, they are a combination of factors that effect readio scores.
If a hex is boosted by a provider, that hex is deemed valuable from an oracle context, and should always be 1x.

Hexes were being "cleaned" of their boost values _before_ trying to determine the oracle multplier, which coupled a hexes boost effect to the assignment multiplier to wether or not the radio was eligible for boosting rewards. 

That was incorrect. Hexes are now "cleaned" at the same time as determining the multipliers.
I could see an argument for the collection being moved to the front as well. But I think either is a better case than in the middle.

I chose the end here because I think it reads easier when testing to have the function, then small decision making structs, _then_ the collection that could be constructed in place.
@michaeldjeffrey michaeldjeffrey merged commit aecb7bc into main Jun 12, 2024
15 checks passed
@michaeldjeffrey michaeldjeffrey deleted the mj/mobile-coverage-points-calculator branch June 12, 2024 18:02
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.

4 participants