Generic scoring

[TomHall2020]

Closes #56

Initial implementation of generic scoring systems for targets. First stage of this PR primarily introduces the concept of a target specification (spec), which is a mapping of target ring sizes in metres to ring scores. This can then be used to calculate expected score using sigma_r and arrow diameter in a very similar way to what was already being done, but with arbitrary inputs.

Hardcoded calculations of expected score are replaced with a calculation from target specifications, and specifications for the supported scoring systems can be derived automatically so existing code is unaffected.

There is not yet a constructor for targets/passes with custom scoring, that its something I wanted to get suggestions on, so this is not a final version by any means.

Other side effects of sorting out the linting/mypy for this include improved type hinting in handicap_equations with type vars, so that functions return type matches the input type rather than being incorrectly hinted as returning the union of floats/arrays of floats. I didn't clean up anything beyond this file but that may allow some other parts of the codebase to be smoother over.

[jatkinson1000]

Will duplicate my comment here from issue #56 now a PR is open:

Yep, I am in support of this.

Comments based on the above:

• It would be good to preserve providing the target type as a string and covering most target types, with user-defined being a fairly specialised option. My guess at the best way to do this would be to have some kind of private dictionary that mapped default face types to instances of your target_specs dictionary.
• Rather than creating a private s_bar function I think it'd be better to just modify arrow_score? But I appreciate having both in place to compare for now.
• With changes in precision we'd need to be careful and see how the tests fare, as the official numbers now set by AGB use the old code - which is the same as the formulation derived by D. Lane. If there are differences then I think there would be a way to wrangle the numbers, it'd just be a bit verbose/ugly. Hopefully this won't be an issue however... I'll think on it.

I am currently working on #58 and #2 so be aware we may have conflict resolution at some point (lets cross that bridge when we get there).
I'll try and take a more detailed look at the code here tomorrow. :)

[jatkinson1000]

Thanks @TomHall2020

The addition of the FloatArray type is great.
However, as you suggest the wider code would benefit from this, so I would consider moving it to consts module. This takes it beyond the scope of this PR, but I may incorporate it as part of #58 or in its own one (unless you want to do so...?).

The code would also improve if you stick the different target specs in a dictionary at the top of the module, and then the long elif structure could be replaced by a single call using spec[system]. I agree this can be simplified further, however, by...

associating spec with the Target class, as it is integral to the target type.
That way these functions can just be passed a Target, greatly simplifying inputs.
I think if a target scoring system is specified as a string (as is the case at present) then the constructor should auto-assign the face_spec attribute from a dict of pre-defined specs (as described in the above paragraph). I would then add the option of passing "custom", in which case the user is also required to provide an optional face_spec argument. How does that sound?

Edit: On reflection below, I think perhaps we should implement a @classmethod called custom_scoring or similar that takes a spec and returns a Target using it. See here for an example.

Thoughts?

The other think I note is that itertools.chain itertools.Pairwise is a python 3.10 feature, so removes the ability to support python v3.9 (EoL Oct 2025). Don't worry about this for now - I'll have a think if it makes sense to drop support of if there is a sensible workaround. It doesn't make sense to just refuse new features that will become mainstream in future.

[jatkinson1000]

Some specific comments in addition to the above:

[TomHall2020]

The addition of the FloatArray type is great. However, as you suggest the wider code would benefit from this, so I would consider moving it to consts module. This takes it beyond the scope of this PR, but I may incorporate it as part of #58 or in its own one (unless you want to do so...?).

I could do a seperate typing PR, also had spotted a few other things before I worked on this that could be cleaned up. Included it in this inittially to keep mypy quiet.
Moving to constants works, although in that case I actually feel like the Length code deserves to be moved to its own units module in that case as it's fairly self contained and will be edited in the future as more units/unit alises are supported.
So happy to do this one seperately, understanding I might ignore some output from mypy that will be fixed by reintroducing FloatArray

The code would also improve if you stick the different target specs in a dictionary at the top of the module, and then the long elif structure could be replaced by a single call using spec[system]. I agree this can be simplified further, however, by...

associating spec with the Target class, as it is integral to the target type. That way these functions can just be passed a Target, greatly simplifying inputs. I think if a target scoring system is specified as a string (as is the case at present) then the constructor should auto-assign the face_spec attribute from a dict of pre-defined specs (as described in the above paragraph). I would then add the option of passing "custom", in which case the user is also required to provide an optional face_spec argument. How does that sound?

Yes! I as soon as I made _get_spec_from_target(target: Target) that really did start to feel like it should be a method on Target itself.

Edit: On reflection below, I think perhaps we should implement a @classmethod called custom_scoring or similar that takes a spec and returns a Target using it. See here for an example.

Thoughts?

So these were the two methods I was going to write up to choose between, handing the spec in the constructor vs having a seperate classmethod. The former makes for a more complex __init__ and is a bit less explicit in what is going on, it also creates more pathways for error handling. The later is probably my first preference.
That also means the Pass constructor needs to be looked at, because at the moment it directly initialises a Target from its own parameters. So the two are linked, a seperate class constructor for target then requires a seperate one for Pass. Will add some samples below.

The other think I note is that itertools.chain itertools.Pairwise is a python 3.10 feature, so removes the ability to support python v3.9 (EoL Oct 2025). Don't worry about this for now - I'll have a think if it makes sense to drop support of if there is a sensible workaround. It doesn't make sense to just refuse new features that will become mainstream in future.
chain is in the itertools docs as far back as 3.5 so no trouble there. The bit that keeps tempting me for 3.10 is replacing the if-else chains with match statements, but thats not really a big deal. 3.9 has 18 months before EOL so think it makes sense to keep supporting for now.

[jatkinson1000]

I could do a separate typing PR, also had spotted a few other things before I worked on this that could be cleaned up.

Happy for that.
Preferred method of working is to open an issue stating what needs doing and why, then open a PR against it.
Keeps things organised and reduces overlap.

In that case I actually feel like the Length code deserves to be moved to its own units module in that case as it's fairly self contained and will be edited in the future as more units/unit alises are supported. So happy to do this one separately, understanding I might ignore some output from mypy that will be fixed by reintroducing FloatArray

Yes, I had also been eyeing up making a units.py module to take it out of constants 😉

So these were the two methods I was going to write up to choose between, handing the spec in the constructor vs having a separate classmethod. The former makes for a more complex __init__ and is a bit less explicit in what is going on, it also creates more pathways for error handling. The later is probably my first preference. That also means the Pass constructor needs to be looked at, because at the moment it directly initialises a Target from its own parameters. So the two are linked, a separate class constructor for target then requires a separate one for Pass. Will add some samples below.

👍

chain is in the itertools docs as far back as 3.5 so no trouble there. The bit that keeps tempting me for 3.10 is replacing the if-else chains with match statements, but that's not really a big deal. 3.9 has 18 months before EOL so think it makes sense to keep supporting for now.

Yeah, realised I made a typo. This is the annoying thing - it doesn't make sense not to use features when they exist and are a great match, but at the same time keeping support can be useful. It's a small use so can worry about it later.

[TomHall2020]

chain is in the itertools docs as far back as 3.5 so no trouble there. The bit that keeps tempting me for 3.10 is replacing the if-else chains with match statements, but that's not really a big deal. 3.9 has 18 months before EOL so think it makes sense to keep supporting for now.

Yeah, realised I made a typo. This is the annoying thing - it doesn't make sense not to use features when they exist and are a great match, but at the same time keeping support can be useful. It's a small use so can worry about it later.

Ah I see what happened here, to clarify for the record its itertools.pairwise that is new in 3.10. We could use this workaround to keep it available in 3.9 for now:

import itertools as itr

def _pairwise(iterable):
        a, b = itr.tee(iterable)
        next(b, None)
        return zip(a, b)

if not hasattr(itr, "pairwise"):
    setattr(itr, "pairwise", _pairwise)

Or just implement it with the tee method and not worry about realiasing, with a note to replace with pairwise when supporting 3.10+

[jatkinson1000]

Yes, I saw that solution when I read up on this.
I agree we shouldn't just use the lowest common denominator, as using pairwise is eventually the best solution long-term, but having both feels a bit clunky. I'll have a think about the best way to implement it. and try to make sure it is flagged for removal in future.

[jatkinson1000]

Resubmitted coverage and looks like you need tests for the returns of the __repr__ methods.
And I'd be happy for you to add a coverage ignore to the python 3.9 workaround, though if I bump to 3.10 soon that will resolve that anyway.

Is this still WIP? Hit the request re-review when you want me to take a closer look.

You also probably want to rebase off of main (and resolve the conflicts) - they don't look too bad.

[TomHall2020]

Yup, still looking to do this. Was seeing how the discussion in #2 shook out, and also going to rebase it off main after #72 gets merged in as that does a more thorough job of preparing the class constructors, reprs etc than I had got done in here.

[TomHall2020]

Right, so this is now up to date and based off main after the big rewrite of #78

Fundamentally the functionality hasn't changed in the latest version here, but to take stock:

• Targets can now provide a face_spec, which is a mapping of ring sizes to scoring zones
• Custom targets can be constructed directly from this specification, or the existing scoring systems can provide it based on their diameter.
• HandicapScheme.arrow_score method now uses this mapping to work out the average arrow score in all cases.
• The various class improvements such as reprs or construtors were implemented already in Class improvements - constructors and reprs #72
• This time I didn't include the tests for consistency with the old implementation; this is done indirectly through the existing test suite on handicaps and classifications.
• _s_bar is still currently its own function but is tested entirely through its public API in arrow_score, similarly to how _get_max_score_handicap is tested through handicap_from_score
  • If we are happy that this is sufficiently well tested and it's still your preference @jatkinson1000 then on this basis I'm happy to inline _s_bar directly into arrow_score
• itertools.pairwise is kept as a workaround for the moment but this will be easy to remove after a future bump to python 3.10 so I don't think its an issue to keep it as is here.
• Docstrings should be fairly up to date at this point but worth a fresh eye over, then I can add examples to the documentation explaining how to use the new features this enables (will probably reuse the kings900 round from the test suite as an example)
• Sneaky side improvement; supported units and scoring systems are now stored directly as class variables on Target which increases DRYness in the methods and makes for easier introspection

[jatkinson1000]

OK, had a proper read of this now and I's very nice.

My main observation is that I think things could be a lot tidier and standardised if it was just expected that every target has a _face_spec.
So as part of the __init__ in the Target class it should just call get_face_spec and assign as appropriate from the scoring system supplied.
This would then allow you to remove the Rings class (and in fact also scoring_system_data) which smells a bit off to me and just use the max/min of _facespec I think?

I would still keep the default constructor as using 'common' names, with from_facespec being an overload.

Few other (mostly smallish) comments/suggestions in the comments.

Check the version of ruff you are running - the ... not being on the same line caught me out previously.

If we merge #82 you can get rid of the faffing for satisfying 3.9 and a couple of other things.

Having had a proper read of the code I'm inclined to say keep _s_bar rather than merging into arrow_score as it keeps things nice and compartmentalised.

[TomHall2020]

OK, had a proper read of this now and I's very nice.

My main observation is that I think things could be a lot tidier and standardised if it was just expected that every target has a _face_spec. So as part of the __init__ in the Target class it should just call get_face_spec and assign as appropriate from the scoring system supplied. This would then allow you to remove the Rings class (and in fact also scoring_system_data) which smells a bit off to me and just use the max/min of _facespec I think?

This makes a lot of sense but does open up one can of worms I haven't dared address yet: mutability.
At the moment, if someone wanted to change the target diameter on an object after creating it, they can. And in the old and current implementation here, it would still give the right results, because previously we relied on the diameter parameter, and currently we regenerate the face_spec from scratch based on the diameter.

Generating the spec on inititalisation and storing makes sense, but we would have to then disallow modification of the target. Easy enough to do if it were a dataclass, I'm not sure what the best approach here would be.

I would still keep the default constructor as using 'common' names, with from_facespec being an overload.

Few other (mostly smallish) comments/suggestions in the comments.

Check the version of ruff you are running - the ... not being on the same line caught me out previously.

Huh, same thing happened, my system ruff ran in place of the virtual enviroment one and it was on 0.22.

If we merge #82 you can get rid of the faffing for satisfying 3.9 and a couple of other things.

Having had a proper read of the code I'm inclined to say keep _s_bar rather than merging into arrow_score as it keeps things nice and compartmentalised.

Ah ok, I missed reading this before commenting, fine with that.

[jatkinson1000]

This makes a lot of sense but does open up one can of worms I haven't dared address yet: mutability. At the moment, if someone wanted to change the target diameter on an object after creating it, they can. And in the old and current implementation here, it would still give the right results, because previously we relied on the diameter parameter, and currently we regenerate the face_spec from scratch based on the diameter.

Generating the spec on inititalisation and storing makes sense, but we would have to then disallow modification of the target. Easy enough to do if it were a dataclass, I'm not sure what the best approach here would be.

Unlikely to really happen, but I agree it should be guarded against/prevented if going down this route.

Using a setter method for diameter might work...?
Add attached behaviour to rescale fecespec values
See https://realpython.com/python-getter-setter/

[TomHall2020]

This makes a lot of sense but does open up one can of worms I haven't dared address yet: mutability. At the moment, if someone wanted to change the target diameter on an object after creating it, they can. And in the old and current implementation here, it would still give the right results, because previously we relied on the diameter parameter, and currently we regenerate the face_spec from scratch based on the diameter.
Generating the spec on inititalisation and storing makes sense, but we would have to then disallow modification of the target. Easy enough to do if it were a dataclass, I'm not sure what the best approach here would be.

Unlikely to really happen, but I agree it should be guarded against/prevented if going down this route.

Using a setter method for diameter might work...? Add attached behaviour to rescale fecespec values See https://realpython.com/python-getter-setter/

I'm brewing on this one. Properties and setters could work but I haven't immediately wrapped my head around how that interacts with the smart behaviour in the constructor (accepting tuple with unit or scalar value).
Option 2 is to rewrite as a frozen dataclass but that gives me serious pause as again I think shoehorning our custom init logic into the dataclass style of doing things is going to be messy.

Another option is immutability lite, I kind of like the approach in this stack overflow answer of "freezing" the object after init by raising an Error from __setattr. It doesn't truly stop someone changing things (that seems to be pretty much impossible), but it would stop someone accidentally changing things, and we can document that the class is meant to not be changed, so if you work around it the behaviour is undefined. Gets a little messy in the classmethod constructor but nothing crazy, have a look at the diff for an idea:

diff --git a/archeryutils/targets.py b/archeryutils/targets.py
index 261d79d..14ad77c 100644
--- a/archeryutils/targets.py
+++ b/archeryutils/targets.py
@@ -92,6 +92,7 @@ class Target:

     """

+    _frozen = False
     _face_spec: FaceSpec

     # Lookup data for min and max scores for supported scoring systems.
@@ -162,6 +163,7 @@ class Target:
         self.distance = distance
         self.native_dist_unit = Length.definitive_unit(native_dist_unit)
         self.indoor = indoor
+        self._frozen = True

     @classmethod
     def from_face_spec(
@@ -221,9 +223,16 @@ class Target:
             }

         target = cls("Custom", diameter, distance, indoor)
-        target._face_spec = face_spec  # noqa: SLF001 private member access
+        object.__setattr__(target, "_face_spec", face_spec)
         return target

+    def __setattr__(self, name, value):
+        """Prevent modification of attributes after initialisation."""
+        if self._frozen:
+            msg = "Cannot modify attributes on Target instance"
+            raise AttributeError(msg)
+        object.__setattr__(self, name, value)
+
     def __repr__(self) -> str:
         """Return a representation of a Target instance."""
         diam, diamunit = self.native_diameter

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.61%. Comparing base (e59da86) to head (879e9bc).

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #59      +/-   ##
==========================================
+ Coverage   97.47%   97.61%   +0.13%     
==========================================
  Files          28       28              
  Lines        1505     1592      +87     
==========================================
+ Hits         1467     1554      +87     
  Misses         38       38              

Files	Coverage Δ
archeryutils/__init__.py	100.00% <100.00%> (ø)
...ils/classifications/agb_outdoor_classifications.py	100.00% <100.00%> (ø)
archeryutils/handicaps/handicap_scheme.py	96.98% <100.00%> (-0.24%)	⬇️
archeryutils/handicaps/tests/test_handicaps.py	100.00% <100.00%> (ø)
archeryutils/length.py	100.00% <100.00%> (ø)
archeryutils/load_rounds.py	77.77% <100.00%> (ø)
archeryutils/rounds.py	100.00% <100.00%> (ø)
archeryutils/targets.py	100.00% <100.00%> (ø)
archeryutils/tests/test_length.py	100.00% <100.00%> (ø)
archeryutils/tests/test_rounds.py	100.00% <100.00%> (ø)
... and 1 more

[TomHall2020]

Changes incorporated, rebased onto main and dealt with the merge conflicts, squashed a few of the tiny commits together while I was rebasing to clean the history up a little bit. Yeah, I really wanted to get this done as well, just didn't quite get it over the line before the weekends selection tournament. Hoping this is the one now.

[jatkinson1000]

Hi @TomHall2020 - grabbing a few morning minutes to get this over the line!

Happy for this to go in now, and thanks for your work and patience.
I'm really pleased with this as, though there look to be a lot of changes, the representation of target faces is now a lot more versatile and clean!