update functions.py

[yucongalicechen]

• Update functions.py: modify the algorithm to select the correct grid point by comparing the two y-coordinates of points on the circle instead of comparing them with the y-coordinate of the grid point.

[yucongalicechen]

• Updated _get_grid_points in functions.py where we prefer to have the grid as a set instead of an array.
• Included test_functions.py with tests functions for get_grid_points and set_distances_at_angle.

[sbillinge]

this looks great, nice job.

• I wonder if we should check intermediate points in the gridding, so go from 3 to 5 on the grid as right now we are only testing points at the center and on the circumference.
• Also for the angle, do we want to test points coming from different quadrants to be sure about the results?
• Please also see my inline comment

The PR is failing CI. Did you sync your main and then merge main into this branch? If you did, then there is something else wrong. You can see by clicking on "details" and see where it failed.

[sbillinge]

I see that you are failing a bunch of pre-commit tests too. This should not happen if the pre-commit is passing on your local. Please run pre-commit install in the top level directory to get this working.

[yucongalicechen]

• For test_get_grid_points: checked grid_size = 3 and 8 (so both odd and even numbers - cases where grid points fall on the circle and fall inside circle).
• For test_set_distances_at_angle: checked distances for acute/right/obtuse angles for grid_size = 3 and 6 and radius = 1 and 0.3. I was going to include some extreme cases like angle = 89.9, but I thought that we're probably not going to calculate distances for these angles, so would this be necessary? Also, although the values of the distances are correct, the order of the distances is a bit weird, I will check about this.
• For test_set_muls_at_angle: values are correct. This probably won't be an issue when we use the function because we will always call set_distances function first, but it seems that I need to add the set_distances function before using set_muls, where there is a set_distances function inside set_muls (i.e., not sure if set_distances inside set_muls is working probably).
• There are two other functions get_path_length and get_entry_exit_coordinates in Class Gridded_circle. They are included in set_distances_at_angle, so I didn't write other tests for them.

[sbillinge]

please see inline comments.

ideally we would have one test per branch and it would be easier to merge these. Just decide if you want to close this PR and do that, or just continue with this PR.

Also, quick question, did you manage to merge upstream main?

[sbillinge]

this set of values seems to be very specific. How did you generate them and how do you know they are correct? A better test is one where you know the right answer and generate without using the function. For an odd number of points use 0.5.

The even number of points specified is harder to test, so do it with a very small number of points, say 2 on each axis and then the points shold presumably be something like 1/3, 2/3.

[sbillinge]

these are easier to read if they are a list of tuples, one for each test, then this tuple has two lists which are for the inputs and outputs. So in this case it would be

params1 = [
    ( [1, 3, 1], [{(...), (...), (...)}, 9] ),
    (...)
]
@pytest.mark.paramaterize("inputs, expected", params1)
def test_...():
    circle = Griddedcircle(inputs[0], inputs[1])

and so on. This is much easier to read.

I may not have the syntax from pytest.mark.paramatrize() quite right so please look it up, but we would like the intent of hte tests to be easier to read in all your tests.

[sbillinge]

this is not how paramatrize works, you don't need this.

[sbillinge]

Decide in your test what behavior you want. Do you care about order? If not, you could use a set, or stick with a list but sort it by value before doing the assert, something like that. If you care about order in the function then test this.

[sbillinge]

you need to run black on everything to get the layout right. But when you install pre-commit it will do it for you when you commit. You will have to run the commit multiple times as when it runs black the commit fails, so just rerun it (if flake8 fails you have to fix the errors manually then rerun the commit).

[sbillinge]

many of these numbers seem magical and too high in precision (is it important for your function that it gets a number correct to 11 significant figures?). Can you manually compute a few special cases and use those as the test? It may be harder here than for the grid points, so just find a reasonable balance, but don't use the function to compute these.

[sbillinge]

remove this for loop.

[sbillinge]

remember, figure out what you want your function to do then write the test to do it. What does this function do and how is it used, and so what behavior do you want in different circumstances, then write the test to check that. Don't be distracted by what the function actually does at the moment. Let's just write the tests first, then we can rewrite any functions to make them have that behavior.

[sbillinge]

I htink this PR can be closed as it is replaced with other PRs. If you agree, please can you close it?