Take only scalar inputs instead of array inputs #50
Conversation
For dipole magnetic fields, request the 3 components of the vector instead of an array.
For the prism code, take both the geometry and the magnetization vector as individual components. Had to update some flake8 config since some functions now take more arguments and are a bit larger.
|
Did all the code conversion and now just need to finish the docstrings. |
|
Awesome work @leouieda! I'll check it out later in depth. Now I'm thinking: since we are operating always with floats, does it makes sense to depend on Numpy? Or in another words, using |
|
Maybe not a performance gain but I wonder if the behaviour of arctan is the same. Also, numba will depend on numpy so maybe it's best if we set explicit versions for it too. |
I made a small experiment running some benchmarks, and the performance gain of |
There was a problem hiding this comment.
I think this is looking good. Thanks for the effort @leouieda!
Probably my main concern is the style. Black is creating this long function calls by putting one argument per line. Usually I agree with Black style, but for these cases, I think it creates too long functions and waste too much horizontal space.
Maybe we could ignore on some of these functions and format them as:
@jit(nopython=True)
def gravity_uu(
easting,
northing,
upward,
prism_west,
prism_east,
prism_south,
prism_north,
prism_bottom,
prism_top,
density,
):
# Return nan if the observation point falls on a singular point.
# For the g_uu this are edges perpendicular to the upward direction
# (parallel to easting and northing)
if (
is_point_on_easting_edge(
easting, northing, upward,
prism_west, prism_east, prism_south, prism_north, prism_bottom, prism_top
)
or is_point_on_northing_edge(
easting, northing, upward,
prism_west, prism_east, prism_south, prism_north, prism_bottom, prism_top
)
): # fmt: skip
return np.nan
# Evaluate kernel function on each vertex of the prism
result = _evaluate_kernel(
easting, northing, upward,
prism_west, prism_east, prism_south, prism_north, prism_bottom, prism_top,
kernel_uu,
) # fmt: skip
# Add 4 pi if computing on the top face to return the limit approaching
# from outside (approaching from the top)
if is_point_on_top_face(
easting, northing, upward,
prism_west, prism_east, prism_south, prism_north, prism_bottom, prism_top,
): # fmt: skip
result += 4 * np.pi
return GRAVITATIONAL_CONST * density * resultLet me know what do you think. Regardless of our opinion on the style, we could totally merge this as it is and then change the style.
|
Hey @santisoler thanks for the review! I did a search for those skip comments but missed those.
Personally I'd rather never manually format even if it looks a bit worse. It's still readable the way black does it and it avoids creating the precedence of introducing these format skip lines. But I'll go with whatever you think is best since I don't feel strongly about this either way. |
|
Let's merge this as it is. If we have strong feelings about the style that black applies, we can always change it in another PR. Great job changing all these functions! |
Update the magnetic forward modelling functions so they pass the prisms boundaries and magnetization vectors to the Choclo functions as scalars. This change was triggered by the modifications introduced in fatiando/choclo#50
Choclo functions shouldn't take array inputs because they may require a user to convert their prism, dipole moment, or magnetization format into what Choclo expects with the possible result of reallocating memory or having to iterate over slices in non-optimal ways. Since these functions are meant to be wrapped by other JIT-compiled functions that the user creates, they should be as flexible as possible with their inputs. This means converting the
dipole_moment,prism, andmagnetizationarguments into individual components instead of arrays. The code is now a bit longer because of this but there is no other down side. Had to update some linting rules to account for the longer code and larger number of arguments that this entails.Relevant issues/PRs:
Fixes #49