Bugfix 630/invalid team

[nbarlowATI]

Issue #630 illustrates the problem that when a real-life player changes team, it is possible for an FPL squad to end up with >3 players from the same team, violating the squad constraints.
The desired behaviour is that the AIrsenal optimization should remove player(s) at the first opportunity, to ensure that the squad is valid.
However:

• If the "starting squad" for a round of optimization is constructed from the Transaction table in the db, rather than being obtained from the API, the "team" attribute of each PlayerCandidate will be the team at the time that player was transferred in.
• Even if the starting squad for optimization has the correct team (i.e. is invalid, with >3 players from the same team), it doesn't necessarily suggest transferring out player(s) to make it a valid team.

This PR does the following:

• Adds update_team method to CandidatePlayer, which retrieves the correct team from the PlayerAttribute for the desired gameweek.
• Adds update method to Squad, which calls update_team for all players in the squad.
• Calls squad.update(gameweek) at the end of get_squad_from_transactions.
  At this point, the starting squad has the correct teams for all players, but more changes needed to enforce transfers that will ensure a valid team:
• Add players_per_team method to Squad that can identify if a squad has too many players from one team.
• Add find_compulsory_transfers function in optimization_transfers.py that uses that information to make a list of players from those teams where we have >3 players, and how many players we need to remove in order to have a valid team.
• Small modifications to make_optimum_single_transfer, make_optimum_double_transfer, make_random_transfers so that they can optionally take a list of players from which to select who to remove (rather than looping over all players in the squad).
• Re-jig the logic and add code in make_best_transfers so that if we are not doing wildcard or free_hit, we first check whether we need to make compulsory transfers, make them if necessary using the functions mentioned above with the list of candidate_players_to_remove, and then subtract the number of compulsory transfers from num_transfers before proceeding with the normal logic.

The above seems to work.... BUT... we are not altering the "strategies" or the number of expected outputs. I think this is fine, except that for strategies with zero transfers in the first gameweek, we will actually be doing a transfer, but it won't be taken into account in the points hit calculation. To me, I think this is a fairly minor and specific problem, and fixing it would add a lot more complication, so it is probably a fair price to pay, but happy to hear other opinions!

[jack89roberts]

I don't think we'd want the 2nd player out to also be limited to candidate_players_to_remove? E.g. if we have 4 City players the best way to solve it might be to make 2 transfers where only 1 of the players transferred out is a City player.

It could get messy but I think we want the first len(candidate_players_to_remove) players out to be limited to players in candidate_players_to_remove, and any subsequent players out to be any player in squad.players?

[jack89roberts]

ah there's some additional logic later on that I missed here, having another think...

[nbarlowATI]

Yep good point! I just simplified the logic in make_best_transfers and changed the logic in make_optimum_double_transfer now, so that:

• It will now do 0, 1, or 2 transfers, as per the strategy.
• If we are doing 1 transfer, and we have a non-empty candidate_players_to_remove list, make_optimum_single_transfer will loop over that list (as before).
• If we are doing 2 transfers, we also pass over num_compulsory_transfers (the number of players we need to remove) to make_optimum_double_transfer. If that number is 1, we only use candidate_players_to_remove for the outer loop, and still loop over the whole squad in the inner loop. If it is 2 or more, we use candidate_players_to_remove for both the inner and outer loops.
• The double-loop logic is a bit more clunky now for making sure we only try each pair of players once (but I think it was plain wrong in the last commit!), but I think it's ok now, for when we loop over the whole squad in both loops (i.e. the normal situation). We actually are trying pairs twice if we're using candidate_players_to_remove in both the inner and outer loops, but the combinatorics are so much smaller in that case that I think it won't slow us down (plus it's a super-rare situation!)

[jack89roberts]

• Even if the starting squad for optimization has the correct team (i.e. is invalid, with >3 players from the same team), it doesn't necessarily suggest transferring out player(s) to make it a valid team.

Is this a bug, I thought the constraint checking in the squad class would take care of everything as long as the teams were correct in the initial squad? As per chat, this is because we only check constraints for the player being added to a squad, not for the whole squad (but we could change that).

The above seems to work.... BUT... we are not altering the "strategies" or the number of expected outputs. I think this is fine, except that for strategies with zero transfers in the first gameweek, we will actually be doing a transfer, but it won't be taken into account in the points hit calculation. To me, I think this is a fairly minor and specific problem, and fixing it would add a lot more complication, so it is probably a fair price to pay, but happy to hear other opinions!

It's valid not to make a transfer I think - if you end up with 4 City players you can keep your squad and score points as normal, but the moment you want to make a transfer it needs to be fixed.