Targetdiameter

[TomHall2020]

Closes #27
Set default target diameter units to cm
Updates tests and load_rounds functionality
Round data files all seem to specify target face sizes in cm already so enforcing this as the default unit makes sense.

Aliases for distance units moved into the constants file as it was getting out of hand and avoiding repitition, choice of conversion constants handled in the branching points in Target.__init__, this could perhaps be handled more elegantly if more units need to be supported in the future but didn't want to change more than necessary for now.

[TomHall2020]

Just saw your message on the issue after posting this, haven't added the units option to Pass yet but would be trivial to add if you want it still.

[jatkinson1000]

Thanks for starting this @TomHall2020, and thanks for getting involved with the project. Progress looks good so far.
I particularly like the condensing of the possible units terms which makes things much cleaner!

I have picked out a few specific comments in the code below, but more generally:

• I would like to have inches as an option (Worcester and IFFA etc.) if you could add this.
  • A knock on from this is that it would probably be a good idea to update the round loader to accept a non-default diameter unit. I could look at this later however if you want.
• it should be possible to pass distance units to the Pass constructor as an optional argument (as you already mention in a comment above).

I'm getting a few spurious additional blank lines and re-arranged imports etc. Are you using an 'intelligent' editor/IDE that has automatically done this?

[TomHall2020]

Updated with Inches support, passthrough of units and I think most of the comments addressed.
I agree that the unit conversion constants are out of hand now. Plus the conversion logic occurs in multiple places now, at least the Target constructor and the Round.get_info method.

I can clean that up either by

1. namespacing into a dict or similar like suggested, and still doing the conversion logic at those call sites
2. factoring the conversion logic out into a seperate function

RE my idea on the call signature for targe and packaging lengths up into their own object, just beacuse if you did want to go down that road then here option 1 is best in this PR and doing that change makes most sense done seperately? Although appreicate its another breaking change to the signature.

Otherwise happy to do 1 or 2 and close this off.

[TomHall2020]

Had another look at this, if you check the latest two commits. First implements namespacing the conversions into a dictionary and getting rid of individual constants CM_TO_METRE etc.

Second takes it a step further and packages everything together into a Length class that is built from the definited constants and contains functions to convert to/from metres and also de-alias unit names. In my opinion it makes the code elsewhere much more readable. If you like the idea then I can add unit tests specifically for that, otherwise can stick with the dictonary approach in the previous commit and just let the current test suite cover it.

[TomHall2020]

While checking the change here noticed a bug, max_distance is comparing across units. A round with mixed distance units can give the wrong result here (eg 75m > 80y, but 80 > 70). Not common, but will come up for anyone making custom rounds, or potentially in IFAA where feet and yards are mixed.

[TomHall2020]

Can include a test and fix in next commits, assuming this was undesired behaviour

[jatkinson1000]

Nice spot, yes this should give the longest distance irrespective of unit of measurement.
Fix appreciated.

[jatkinson1000]

Yep, like the idea of a Length class. Couple of thoughts on the finer implementation that I will add in-code comments for.

Also dropped a comment on the 'magnitude and units' conversation.

Finally - you may want to run black on the code pre-submission.

[TomHall2020]

Think this should be pretty much there now.

[jatkinson1000]

Thanks @TomHall2020 this is amazing!

I added a couple of commits for nits (standardise 'diam' to 'diameter') and to update the examples notebook but I agree this is all ready to go 🚀

Thanks again for contributing, it's great to see my code being used and others take an interest and you are welcome to contribute again in future. You write well, but also apply good SE principles, and hopefully we both learnt some new things. 😄

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (581fed0) 95.07% compared to head (c8f4ba9) 95.38%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
+ Coverage   95.07%   95.38%   +0.31%     
==========================================
  Files          22       23       +1     
  Lines        1218     1301      +83     
==========================================
+ Hits         1158     1241      +83     
  Misses         60       60              

Files	Coverage Δ
archeryutils/constants.py	100.00% <100.00%> (ø)
...cheryutils/handicaps/tests/test_handicap_tables.py	100.00% <100.00%> (ø)
archeryutils/handicaps/tests/test_handicaps.py	100.00% <ø> (ø)
archeryutils/load_rounds.py	74.62% <ø> (ø)
archeryutils/rounds.py	100.00% <100.00%> (ø)
archeryutils/targets.py	100.00% <100.00%> (ø)
archeryutils/tests/test_constants.py	100.00% <100.00%> (ø)
archeryutils/tests/test_rounds.py	100.00% <100.00%> (ø)
archeryutils/tests/test_targets.py	100.00% <100.00%> (ø)