3

I have this template tag that ultimately returns a list of 'active' advertisements (checks if Campaign with the active field is True, then pulls ads from the Campaign with a queryset)

@register.assignment_tag
def get_current_campaigns(amount):

    # Get all the campaigns that are active 
    current_campaigns = Campaign.objects.filter(active=True)

    current_campaigns_count = current_campaigns.count()

    # To avoid the list index being out of range and throwing an IndexError
    # We reduce the amount to match the amount of rows in the model if the
    # amount of rows is less than the amount being requested.
    if amount > current_campaigns_count:
        amount = current_campaigns_count

    # Select active campaigns randomly
    random_camps = []
    for i in range(amount):
        random_camps.append(random.choice(current_campaigns))

    # prepare all the ads to return 
    output = []
    for campaign in random_camps:
        # get all the ads that a campaign has 
        ads = campaign.advertisement_set.all()
        # now select one randomly
        ad = random.choice(ads)
        # hand it to output 
        output.append(ad)
        # mark that this campaign has been seen
        campaign.impressions = F('impressions') + 1
        campaign.save()
        # checks and sets if the campaign is still active
        campaign.check_active()

    return output

And here is the Model that goes with it:

class Campaign(models.Model):
    ''' Represents an Advertisement Campaign '''
    title = models.CharField(max_length=50, blank=True, verbose_name='Campaign Title')
    impressions = models.IntegerField()
    impression_limit = models.IntegerField()
    created = models.DateTimeField(auto_now_add=True)
    active = models.BooleanField(default=False)

    def check_active(self):
        ''' Checks if the Campaign is currently active '''
        if self.impressions >= self.impression_limit:
            self.active = False
            self.save()

The strange bit: Every time I visit the page the ad is on and then check it in the admin, the object impressions goes up by 2 (it should be 1) and gets marked as False, even if this if self.impressions >= self.impression_limit isn't true, it still somehow changes the active field to being False regardless.

Any clue why this strange behavior is happening? I can provide more info if needed.

ApathyBear
  • 9,057
  • 14
  • 56
  • 90

1 Answers1

4

random.choice does not guarantee to produce non-repeating items.

import random

random_camps = random.sample(current_campaigns, amount)

is the way to go here.

Update If you're worried about the speed, this question addresses quick random row selection in postgres.

Community
  • 1
  • 1
axil
  • 1,558
  • 16
  • 17
  • This would throw an attribute error wouldn't it? Shuffle is not an attribute of 'QuerySet' – Llanilek May 24 '15 at 17:56
  • Interesting, just how efficient would this be? I mean if current_campaigns ends up being a large queryset for example? – Llanilek May 24 '15 at 18:37
  • and more to the point, how does this guarantee to produce non-repeating items? – Llanilek May 24 '15 at 18:39
  • Internally it uses just the same random number generator as any other `random.*` function, so I would expect it to be just as fast as manually generating the sequence and checking it for non-repeatability (eg using a `set()` of already generated items). – axil May 24 '15 at 18:47
  • Shuffle (just as any other pseudo-random based approach) has another problem - due to limited length of nonrepeating sequence of random numbers it wont reach all possible permutations. – axil May 24 '15 at 18:48
  • `choice` selects any item from the sequence without caring what was selected earlier. That leads to repeated items. `shuffle` on the other hand generates a *permutation*. It guarantees that each item from the initial sequence will be in the result and it there will be only one copy of it. That's the definition of permutation. – axil May 24 '15 at 18:49
  • iterating over the amount and appending the shuffled list into a list gives us [None, None] if the amount = 2. Could you display in your example how shuffle would work in this example? because running shuffle on the list just changes the order of the list. – Llanilek May 24 '15 at 18:56
  • fixed, have a look now – axil May 24 '15 at 19:09