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

Unskip and rename test_expression_metric #8578

Merged

Commits on Sep 6, 2023

  1. Add docstrings to contracts/graph/metrics.py functions to document …

    …what they do
    
    Used [#5607](#5607)
    for context on what the functions should do.
    QMalcolm committed Sep 6, 2023
    Configuration menu
    Copy the full SHA
    1ac2f04 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    b84b575 View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    a92e5aa View commit details
    Browse the repository at this point in the history
  4. Add typing to base_metric_dependency and `derived_metric_dependency…

    …` and update functions to work on 1.6+ metrics
    QMalcolm committed Sep 6, 2023
    Configuration menu
    Copy the full SHA
    c8389f9 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    8f0c5c6 View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    4e062cb View commit details
    Browse the repository at this point in the history
  7. Configuration menu
    Copy the full SHA
    4dfe539 View commit details
    Browse the repository at this point in the history
  8. Simplify conditional controls in ResolvedMetricReference functions

    The functions in `ResolvedMetricReference` use `manifest.metric.get(...)`
    which will only return either a `Metric` or `None`, never a different
    node type. Thus we don't need to check that the returned metric is
    a metric.
    QMalcolm committed Sep 6, 2023
    Configuration menu
    Copy the full SHA
    4a11c9f View commit details
    Browse the repository at this point in the history
  9. Don't recurse on over depends_on for non-derived metrics in `revers…

    …e_dag_parsing`
    
    The function `reverse_dag_parsing` only cares about derived metrics,
    that is metrics that depend on other metrics. Metrics only depend on
    other metrics if they are one of the `DERIVED_METRICS` types. Thus
    doing a recursive call to `reverse_dag_parsing` for non `DERIVED_METRICS`
    types is unnecessary. Previously we were iterating over a metric's
    `depends_on` property regardless of whether the metric was a `DERIVED_METRICS`
    type. Now we only do this work if the metric is of a `DERIVED_METRICS`
    type.
    QMalcolm committed Sep 6, 2023
    Configuration menu
    Copy the full SHA
    41c8ad0 View commit details
    Browse the repository at this point in the history
  10. Configuration menu
    Copy the full SHA
    b476f28 View commit details
    Browse the repository at this point in the history
  11. Configuration menu
    Copy the full SHA
    84c5091 View commit details
    Browse the repository at this point in the history
  12. Configuration menu
    Copy the full SHA
    b073cff View commit details
    Browse the repository at this point in the history
  13. Configuration menu
    Copy the full SHA
    711ca98 View commit details
    Browse the repository at this point in the history
  14. Configuration menu
    Copy the full SHA
    70091df View commit details
    Browse the repository at this point in the history
  15. Configuration menu
    Copy the full SHA
    da99e59 View commit details
    Browse the repository at this point in the history

Commits on Sep 7, 2023

  1. Move from manifest.metrics.get to manifest.expect in metric helpers

    Previously with `manifest.metrics.get` we were just skipping when `None`
    was returned. Getting `None` back was expected in that `parent_unique_id`s
    that didn't belong to metrics should return `None` when calling
    `manifest.metrics.get`, and these are fine to skip. However, there's
    an edgecase where a `parent_unique_id` is supposed to be a metric, but
    isn't found, thus returning `None`. How likely this edge case could
    get hit, I'm not sure, but it's a possible edge case. Using `manifest.metrics.get`
    it we can't actually tell if we're in the edge case or not. By moving
    to `manifest.expect` we get the error handling built in, and the only
    trade off is that we need to change our conditional to skip returned
    nodes that aren't metrics.
    QMalcolm committed Sep 7, 2023
    Configuration menu
    Copy the full SHA
    f80b4e1 View commit details
    Browse the repository at this point in the history