Refactor the static methods to functions in fingerprint

[drvinceknight]

There are 4 static methods in the fingerprint class. As they are methods they cannot be tested in isolation (you need to create the class first):

• create_jossann
• create_edges
• generate_data
• reshape_data

This issue would be to pull those methods out in to their own functions and adjust/isolate their tests.

[eric-s-s]

i've got the tests 1/2 written. so i'll be happy to do it.

[drvinceknight]

👍 Thanks :D

[eric-s-s]

i'll have this done in a day or so, but these tests might be worth revisiting once #885 is completed. as they rely on str(player).

[drvinceknight]

these tests might be worth revisiting once #885 is completed.

Because you're testing equality of the probes?

I'm thinking of attacking #885 myself. I've got all the code in place, so it's just a question of putting it in place. The work that was being done on it seems to have stalled.

[eric-s-s]

Because you're testing equality of the probes?

exactly!

[drvinceknight]

I'm just finishing something off and then I'll submit a PR for #885. Shouldn't take me long (famous last words).

[eric-s-s]

do you prefer the pulled out functions to be above or below the main class? if below, should create_points be pulled down below the main class too?

[eric-s-s]

I'd like to add create_probes to the list of things to pull out of the class. It becomes static once create_jossann is out of the class.

[drvinceknight]

do you prefer the pulled out functions to be above or below the main class? if below, should create_points be pulled down below the main class too?

I have no indication as to whether or not there's a best practice. I think if I was doing it I'd put it above...

I'd like to add create_probes to the list of things to pull out of the class. It becomes static once create_jossann is out of the class.

Sounds sensible 👍

[drvinceknight]

#885 is completed.

Have opened #975

[eric-s-s]

also will do typehinting as per #808

[eric-s-s]

@drvinceknight

I have no indication as to whether or not there's a best practice. I think if I was doing it I'd put it above...

if it really is 6 of one, half a dozen of the other, i'll submit in a top-down style (main class, then supporting functions) and if it looks bad, it's just a matter of cut and paste

[eric-s-s]

any issue with removing the progress bar from create_points and create_edges. The slow ones are create_probes and SpatialTournament.play().

create_points and create_edges are effectively list comprehensions and too short to see a progress bar. here are the timeits for creating a million-point square and a million edges. and no one would accuse me of having a fast computer. (although not painfully slow, either)

>>> %timeit [(x*0.001, y*0.001) for x in range(1000) for y in range(1000)]
1 loop, best of 3: 332 ms per loop
>>> %timeit [(0, probe_index) for probe_index in range(1, 1000000)]
10 loops, best of 3: 137 ms per loop

even on my god-awful, painfully slow, terrible linux machine a million points take 2 and 1 seconds. respectively. and that thing is slower than my 6-year-old samsung phone.

[drvinceknight]

any issue with removing the progress bar from create_points and create_edges. The slow ones are create_probes and SpatialTournament.play().

I would like them to stay. They can always be turned off by a user.

Let's keep this issue about simply moving the static methods out in to private functions and not change any functionality.

[drvinceknight]

@eric-s-s I think I caught a few of them in #972 (which is now in) but in general run the tests with progress_bar=False to keep testing output as clean as possible. There will also be some that have to stay obviously for testing purposes that the progress bar actually work (as is currently the case) but if you see any that can be suppressed that we've missed please turn them off :)

[eric-s-s]

btw - i move these lines out into a function to be able to explicitly test them. (and found a warning on unclosed file in the process.)

def update_according_to_os(filename: Union[str, None],
                           in_memory: bool) -> Tuple[str, bool]:
    """Adjust filename and in_memory according to O.S."""
    if axl.on_windows and filename is None:  # pragma: no cover
        in_memory = True
    elif filename is None:
        with NamedTemporaryFile(mode='w') as file:
            filename = file.name
    return filename, in_memory

[drvinceknight]

Nice! 👍

[drvinceknight]

Although I wonder if that file needs to stay open? I expect that warning didn't happen when that file was being used directly in the test when it was part of the other method that actually wrote to it.

If you are going to take that function out I think it needs a better name than update_according_to_os (because that's not technically the only thing it's doing, it's not just dependant on the OS). This might be worth leaving as is for the purpose of this issue: I think it's fine where it was.

[drvinceknight]

Although I wonder if that file needs to stay open?

Yes indeed (just checked) it's being closed properly by the tournament when used by it. Make sense that the warning would trigger when it's pulled out though.

Let's try to stick to moving those static methods for this issue please.

[drvinceknight]

Closed by #984