fatiando / harmonica

Forward modeling, inversion, and processing gravity and magnetic data
https://www.fatiando.org/harmonica
BSD 3-Clause "New" or "Revised" License
216 stars 70 forks source link

Error in the test_prims_layer #344

Open aguspesce opened 2 years ago

aguspesce commented 2 years ago

I'm making the tesseroid_layer test and reading the test_prims_leyer. So, I think that I noted an error when check if a layer of prisms is property constructed in line 96. The original line is:

expected_top[1, 1], expected_bottom[1, 1] = reference, surface[1, 1]

But I think that it should be:

expected_top[1, 1], expected_bottom[1, 1] = reference[1, 1], surface[1, 1]

@santisoler... am I right?

santisoler commented 2 years ago

Thanks for opening this issue @aguspesce.

For the record, you are talking about this line: https://github.com/fatiando/harmonica/blob/d7c43540199a787246557afb8a20e071569c0f48/harmonica/tests/test_prism_layer.py#L96

The reference variable is defined in line 82: https://github.com/fatiando/harmonica/blob/d7c43540199a787246557afb8a20e071569c0f48/harmonica/tests/test_prism_layer.py#L82

And the dummy_layer is the return of the dummy_layer fixture: https://github.com/fatiando/harmonica/blob/d7c43540199a787246557afb8a20e071569c0f48/harmonica/tests/test_prism_layer.py#L32-L48

As you can see, the reference variable is either a float or an xarray.DataArray with a single value, so line 96 is actually ok. If I would made your proposed change, I would get an error because the float doesn't have a [1, 1] element, it cannot be indexed.

Maybe defining the reference as an xarray.DataArray with a single value is confusing and misleading. We could change that if that's make the tests more readable.


Since this is not an actual bug, I'm removing the bug label from it.