Codecov Report

Attention: Patch coverage is 63.01370% with 27 lines in your changes missing coverage. Please review.

Project coverage is 96.41%. Comparing base (99fe0a9) to head (a45254d).

@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
- Coverage   97.81%   96.41%   -1.40%     
==========================================
  Files          30       32       +2     
  Lines        1742     1815      +73     
==========================================
+ Hits         1704     1750      +46     
- Misses         38       65      +27     

but this also points to a deeper issue I'll get to later

As promised, I think fundamentally the fact that everything going into these classification functions aside from a score is a string, is a serious issue.

Both in terms of what has to happen internally in the code to process these strings:

#Theres lots of string processing and mangling going on to clean up the arguments
/archeryutils/classifications/agb_old_field_classifications.py:
  178:     if age_group.lower().replace(" ", "") in ("adult", "50+", "under21"):
  179          age_group = "Adult"
  180:     elif re.compile("under(18|16|15|14|12)").match(age_group.lower().replace(" ", "")):
  181          age_group = "Under 18"

  190      if (
  191:         bowstyle.lower().replace(" ", "") in ("compound", "recurve")
  192          and "wa_field_24_red_" not in roundname
  193      ):
  194          return "unclassified"
  195      if (
  196:         bowstyle.lower().replace(" ", "")
  197          in (
  198              "barebow",

...

And also for the end user as they are trying to figure out how to use the functions

# you are utterly on your own when you are trying to find the right round name or category names...
In [6]: import archeryutils.classifications as cf

In [7]: cf.agb_outdoor_classification_scores('short warwick', 'recurve', 'womens', 'u18') #wrong round, wrong gender, wrong age class...
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
Cell In[7], line 1
----> 1 cf.agb_outdoor_classification_scores('short warwick', 'recurve', 'womens', 'u18')

File ./archeryutils/classifications/agb_outdoor_classifications.py:543, in agb_outdoor_classification_scores(roundname, bowstyle, gender, age_group)
    540     bowstyle = "Compound"
    542 groupname = cls_funcs.get_groupname(bowstyle, gender, age_group)
--> 543 group_data = agb_outdoor_classifications[groupname]
    545 # Get scores required on this round for each classification
    546 class_scores = [
    547     hc.score_for_round(
    548         group_data["class_HC"][i],
   (...)
    553     for i in range(len(group_data["classes"]))
    554 ]

KeyError: 'u18_womens_recurve'

Finally you end up with the frustating situation that if you have already constructed a round object from scratch, or even loaded one from the pre-assigned rounds. You can't actually use it to work out classifications, even though the classification functions will use a fully instantiated round object to do thier own work.

from archeryutils import load_rounds
rnd = load_rounds.AGB_indoor.portsmouth
cf.agb_indoor_classification_scores(rnd, 'compound', 'male', 'under 14')

# TypeError: Unhasable type: 'Round'

It is, I think, a bit too far beyond the scope of what I want to get done in this request. But is something I'm keeping in mind as I work on it.

@TomHall2020 a lot to unpack, so will try and place comments here, but please remind me if I miss something.

As you already raise, this should remain a PR for reviving old outdoor classifications, with other amendments made through other pull requests please.

These could include:

• Use of itertools.product() Yep, not come across this before, so useful to learn about. Looks like it should do the job of flattening those loops. WIP in Data looping in classification dict generation functions #110
• Standardising old field classifications to return UC. I think this came about from handling compatibility in archerycalculator. I have patched it in Old field classifications return words not codes for classifications #104 -> Return codes for old field classifications #105
• Naming of old agb field functions to be consistent with old indoor. Patched in Update old field classification naming #107
• Moving to use cf rather than class_funcs. I think I am in agreement with this in principle. Is this only a documentation and notebook change?
• Documentation patches that are spotted.

Other points:

• under 18 vs. 50+ was to match the WA/ArcheryGB terminology (and this was after a midway change from master)... Would need some thought on what is most sensible - there is an argument for sticking with the WA/AGB terminology that users would expect.
• That said, it would be useful to have some way of easily checking what the available options are, as this has caught be before and I agree it is irritating digging back around to remind yourself what to use.
• IIRC the reason for not using a Round object was because each classification scheme is restricted to a set of named rounds, and I enforce this by checking that the name appears in the dict of rounds instantiated in each classification module. I suppose one could pass in a Round and check that it's name appears in this list and I can perhaps see where this might come in useful, but could this not also be resolved by passing Round.name to the classification function?

For the linting issues you are seeing I have submitted a patch in #106 which you can rebase off of once merged.

Use of itertools.product() Yep, not come across this before, so useful to learn about. Looks like it should do the job of flattening those loops.

In the spirit of seperating things out I'll do a quick PR for that once the other small ones are merged in, and then I can rebase the next steps for this on top of everything else. Should be a trivial change as its just an implementation detail, no changes to docs/testing required.

Moving to use cf rather than class_funcs. I think I am in agreement with this in principle. Is this only a documentation and notebook change?

And using the same convention where the package is imported into test files as well.

under 18 vs. 50+ was to match the WA/ArcheryGB terminology (and this was after a midway change from master)... Would need some thought on what is most sensible - there is an argument for sticking with the WA/AGB terminology that users would expect.

Huh, I hadn't really clocked that was the case but right you are. Example Ianseo results page. Having said that I don't know if most people will realise that there is that distinction, and at least at WA events it is also very common to see the abbreviated U21, U18 as well on schedules, target lists etc. for example Nimes.

That said, it would be useful to have some way of easily checking what the available options are, as this has caught be before and I agree it is irritating digging back around to remind yourself what to use.

At a minimum it needs to be documented, at the moment you can only find out from the souce code itself, and due to the way the data dictionaries are constructed even then the only place that actually contains all of the supported age category strings in a single file is the json data file.

Having said all that it seems a bit silly to put too much effort into deciding which nomenclature is the most correct because I think its clear that the best thing to do is to support common aliases at the point of use, so long as the input is unambigous. We aren't really here to catch users out.

I'm wondering if this is a use case for an Enum? That would be more self documenting and allows autocompletion. I'm fairly sure with the right use of StrEnum we could make it so that providing the old string values or valid aliases that we pre-define also still passes so it remains backwards compatible.

I'll open an issue on the topic of supporting aliases, and list some suggestions as to which ones could be included. Then any implementation details could follow on from that.

IIRC the reason for not using a Round object was because each classification scheme is restricted to a set of named rounds, and I enforce this by checking that the name appears in the dict of rounds instantiated in each classification module. I suppose one could pass in a Round and check that it's name appears in this list and I can perhaps see where this might come in useful, but could this not also be resolved by passing Round.name to the classification function?

Yeah, I did guess enforcing supported rounds per system was the reason. Passing round.name works fine, but on principle of least surprise it still seems frustrating to have to parse out my roundname from my perfectly functional round object just so it can be looked up again. I'd argue that given the fix of looking up the rounds name is so simple, why not just allow the functions to accept a round and we do that check as well? Could be abstracted to a utility function.

#inside xx_classification_scores()
rnd = get_check_round_by_name(roundname, ALL_XX_ROUNDS)
...

def get_check_round_by_name(rnd, accepted_rounds)
if isinstance(rnd, Round) and rnd.name in accepted_rounds
    return rnd
else:
    return accepted_rounds[rnd]

Naming feels a bit clunky but the principle is there.

For the linting issues you are seeing I have submitted a patch in #106 which you can rebase off of once merged.

D'oh, once again caught out by linter versioning. I actually hadn't updated my venv with the changes to main since my last contributions.

On mentioning the linting, I'm pre-empting making the generic classification functions and related to the talk about string arugments to the underlying methods, we are going to get complaints from the linters about having too many function arguments when implementing those, and to be fair they do seem right. An example will be clearer:

# current example from agb_outdoors:
def calculate_agb_outdoor_classification(
    score: float,
    roundname: str,
    bowstyle: str,
    gender: str,
    age_group: str,
) -> str:
    ...

# to make generic we now also need to take 
# the handicap system (old or new) 
# and discipline/format (indoor/outdoor/field)
# as parameters

def calculate_classification(
    score: float,
    roundname: str,
    bowstyle: str,
    gender: str,
    age_group: str,
    handicap_system: str,
    discipline: str
) -> str:
    ...

# ERROR: PLR0913 Too many arguments in function definition (7 > 5)

I agree it is too many, they are nearly all strings so type safety is pretty useless, and would be pretty unwieldy to use:

>>> calculate_classification(1200, 'york', 'recurve', 'female', '50+', 'AGB', 'outdoor') 
# works
>>> calculate_classification(1200, 'york', 'female', 'recurve', '50+', 'AGB', 'outdoor')
# oops wrong order of args, mypy can't catch this

This ordering one especially bites me because while it matches the order the composite keys that get_groupname produces, it doesn't align with the order I would say it in my head which is probably one of either "recurve 50+ women" or "womens recurve 50+".

Accepting preformed group names such as"under15_male_recurve" is no good because:

1. Makes it extra work for the consumer who now has to do thier own function call or else they will try to hack the group name together themselves
2. We sometimes do business logic such as transforming the age categories or bowstyles before getting the group name

Perhaps one way to cheat the linter would be to take those as a (named)tuple rather than seperate arugments:

>>> calculate_classification(1200, ('recurve', 'female', '50+'), 'AGB', 'outdoor')
# smallest change from existing format, just need to enclose the category arguments in parentheses
>>> category = Category(gender='female', bowstyle='recurve', age='50+')
>>> calculate_classification(1200, category, 'AGB', 'outdoor')
# now we can also encapsulate the category information and pass arguements out of order by keyword

I was remembering how we handled the parsing of units as tuples in the target/pass/round functionality. Food for thought.

@TomHall2020 I have just added the handicap thresholds for the various categories.

I pushed them direct to this branch (couldn't get GitHub to play nice with opening a PR from my repo to your fork) so you'll want to pull to get these locally before any further changes to avoid conflicts.

More generally if you rebase off of the latest main on my repo that should hopefully resolve the conflicts and linting issues.

For tests there is a version of the old classification tables available here: https://southleedsarchers.org.uk/wp-content/uploads/2022/03/AGB-Shooting-Admin-Procedures-2020.pdf