Comparison of values in dictionary

Question

I have the following dictionary:

results_dict = {'Current model': {'Recall': 0.77,
  'Precision': 0.1,
  'F1_score': 0.18,
  'Frauds': 94},
 'New model': {'Recall': 0.96,
  'Precision': 0.17,
  'F1_score': 0.29,
  'Frauds': 149}}

What I want is to make a comparison for each metric between the two models, and in case the one belonging to the new model is better, adding +1 to a variable i. So far, I have made this:

recall_new = results_dict['New model']['Recall']
recall_current = results_dict['Current model']['Recall']
precision_new = results_dict['New model']['Precision']
precision_current = results_dict['Current model']['Precision']
F1_new = results_dict['New model']['F1_score']
F1_current = results_dict['Current model']['F1_score']
frauds_new = results_dict['New model']['Frauds']
frauds_current = results_dict['Current model']['Frauds']

i = 0

if recall_new > recall_current or recall_new > (recall_current-(0.1)):
    i+=1
if precision_new > precision_current or precision_new > (precision_current-(0.1)):
    i+=1
if F1_new > F1_current or F1_new > (precision_current-(0.1)):
    i+=1
if frauds_new > frauds_current or frauds_new > int(round(frauds_current*1.2,0)):
    i+=1

The code makes what is intended to do but it is very verbose and repetitive and I was wondering whether it could be simplified or not. Thank you!

Answer 1

Index less

Also, you're indexing a lot. Perhaps it's better to seperate the two dicts:

old, new = results_dict["Current model"], results_dict["New model"]

(And what's up with having an underscore in Current_model and a space in New model? That's asking for typos. You're not even consistent with it - in your first code they're both spaces, but in the assignment you use 3 underscores and a space for Current...)

Also, with how you make the checks, one of your conditions always implies the other. You should remove the redundant comparisons. Change:

if recall_new > recall_current or recall_new > (recall_current-(0.1)):
# Into:
if recall_new > recall_current - 0.1:

The additional braces don't do anything, and if recall_new is bigger than current-0.1, then it is also bigger than current.

Loop

If you look closely, you'll see you're doing the same thing multiple times. So just make it a loop.

Arguably you should make an outside variable for the keys to iterate over, or iterate over the keys of either new or old dict. But if hard-coding is appropriate, it could look a lot like:

new_better = 0  # i is a bad variable name. Names should have meaning.
for key in ("Recall", "Precision", "F1_score"):
    if new[key] > old[key]-0.1:
        new_better += 1
if new["Frauds"] > old["Frauds"]*1.2:
    new_better += 1

Note that I removed your rounding. Python has no issues transforming a float to the closest integer by means of int(), but neither does it have a problem with comparing ints to floats. I did notice that your adjustments make it easier to score an increment for the first three variables, but harder for the fourth. Is this intentional?

Answer 2

I'm not convinced your code is too long, but it is verbose in the sense that it's a bit hard to read. To make sense of it one must read many lines in detail and identify the repeated pattern between them. Wrapping things in functions can improve clarity just as much as using a more concise syntax or pattern

Gloweye's concern about your choices of dictionary keys is sound. I'd go further and suggest that this is a good time to write a small class.

There are different ways to think about classes and objects. Without getting too deep into the weeds, the thing they offer us here is the ability to express the structure of a "dictionary" variable in our code.

from typing import Dict, Union

class MyModel:
    def __init__(self, recall: float, precision: float, f1: float, frauds: int):
        self.recall = recall
        self.precision = precision
        self.f1 = f1
        self.frauds = frauds

    def from_dict(data: Dict[str, Union[int, float]]) -> 'MyModel':
        return MyModel(data['Recall'], data['Precision'], data['F1_score'], data['Frauds'])

    def recall_surpassed(self, new: float) -> bool:
        return new > self.recall - 0.1

    def precision_surpassed(self, new: float) -> bool:
        return new > self.precision - 0.1

    def f1_surpassed(self, new: float) -> bool:
        return new > self.f1 - 0.1

    def frauds_surpassed(self, new: float) -> bool:
        return new > self.frauds

    def get_improvement_score(self, new: 'MyModel') -> int:
        return (
            int(self.recall_surpassed(new.recall))
            + int(self.precision_surpassed(new.precision))
            + int(self.f1_surpassed(new.f1))
            + int(self.frauds_surpassed(new.frauds))
        )

This isn't any more concise than what you'd written, but here the verbosity serves a purpose: it's easier to make sense of the behavior and to find any particular detail because the pieces are split up and labeled. For example, did I get the frauds check right, and if I didn't then how would you fix it?

To use this with your existing nested dicts would be easy enough, because I included from_dict as a helper method:

i = MyModel.from_dict(
    results_dict['Current model']
).get_improvement_score(
    MyModel.from_dict(results_dict['New model'])
)
print(i)

Answer 3

As others have pointed out, your comparison operations are skewed between the Frauds fields and all other fields. Given that the data type appears to be different, I'm going to assume that you're doing the comparison correctly, and it's working as intended.

That said, the numbers you are using to subtract (0.1) appear surprisingly large in relation to the values you are comparing them with. In one case, your initial value is 0.1, so subtracting 0.1 would result in comparing the new value > 0, which might not be what you intended.

Iterating the keys

You can use the dict.keys() iterable to get all the keys for one of the dictionaries, and you can use Python's conditional expressions to evaluate your score.

Combine those together and you get:

def score_results(old, new):
    """ Return a score based on the number of 
        'improved' fields of the new results over 
        the old.
    """
    score = 0

    for k in new.keys():
        if k == 'Frauds':
            score += 1 if new[k] > int(old[k] * 1.2) else 0

        else:
            score += 1 if new[k] > old[k] - 0.1 else 0
            # NOTE: did you mean > old[k] * 0.9 ???

    return score

Lambdas and defaults and iterables, oh my!

You can shorten this by putting your brain in Python-mode and treating code as data using Python's first-class functions. In this case, we'll make use of the lambda expression syntax, since the things we're doing are so short:

def score_results(old, new):
    """ Idem. """

    minor_change = lambda o: o - 0.1  # could be o * 0.9??
    change_funcs = { 'Frauds': lambda o: int(o * 1.2) }

    return sum((1 if new[k] > change_funcs.get(k, minor_change)(old[k]) else 0) 
               for k in new.keys())

In this version, I used the conditional-expression syntax from above to evaluate either 0 or 1 for each key k. I used the sum() built-in to add up the scores. This replaces the for k in new.keys() loop with an iterable. The iterable I chose was the generator expression that looped over the k in new.keys().

I could have used an if clause in the generator expression to skip over the Frauds key. But we don't want to skip it, we want to compute it differently. So instead I built a dictionary where I could look up the keys. Every key would have a default behavior, except for special keys, by using the dict.get(key, default) method. The special keys in this case are Frauds:

change_funcs.get(k, minor_change)

Once I had the special function (for Frauds) or the default function (minor_change for everything except Frauds) I could call it:

change_funcs.get(...)(old[k])

And then put it into the comparison with the new key as part of the conditional-expression.

int(bool) -> {0, 1}

Another "shortening" that could be made is to note that Python converts Boolean values to integers by mapping False to 0 and True to 1. So instead of the conditional expression:

1 if cond else 0

we could simply use:

int(cond)

This converts our sum expression to:

    return sum(int(new[k] > change_funcs.get(k, minor_change)(old[k])) for k in new.keys())