harmonica
harmonica copied to clipboard
Change default for window size in EquivalentSourcesGB
Change the default for the window size in the EquivalentSourcesGB constructor. By default, approximately 5000 data points are in each window.
Relevant issues/PRs: Fixes #425
- [x] Add a test to check window size default. Use numpy.testing.assert_allclose
- [x] Raise an warning for no. of points < 5e3
Completed with @santisoler
This is starting to look great @indiauppal!
I'm leaving a few ideas after the meeting we had today:
- We should probably extend the test function for less than 5000 points and test if the outputs match the ones for a single window. Something like:
data_windows, source_windows = eqs._create_windows(grid_coords) # The output lists should have a single element each (corresponding to the single window) assert len(data_windows) == 1 assert len(source_windows) == 1 # Check if all sources and data points are inside the window for coord in grid_coords: npt.assert_allclose(coord, coord[data_windows[0]]) for coord in eqs.points_: npt.assert_allclose(coord, coord[source_windows[0]]) - We could explore replacing those
np.aranges withslices. We only need those indices to slice the sources and coordinates arrays in theself._gradient_boostingmethod, so anything we could use to slice them that are more memory efficient should work. This is just a minor optimization, so it's not that critical for this PR. - Since now we have another attribute the
fitmethod assigns (thewindow_size_), we should add it to the list ofAttributesin the class docstring. Something like the think I wrote in #491 for thedepth_argument: https://github.com/fatiando/harmonica/blob/55309c781e0059f78ced658c80d049ea222f31e4/harmonica/_equivalent_sources/cartesian.py#L142-L145 - It would be nice to add a check for the
window_sizeargument in the__init__method. Basically, we should raise an error if the passed value is astrand is not"default". That's the only string value that should be valid. Check this out for inspiration: https://github.com/fatiando/harmonica/blob/55309c781e0059f78ced658c80d049ea222f31e4/harmonica/_equivalent_sources/cartesian.py#L176-L180
This is somewhat my personal wishlist for this PR, so feel free to assign me a few of these tasks if you want. As always, feel free to ask for help if you need it 🙂
Looking forward to see this merged!
@indiauppal, I'm updating this branch after the fix I made for the failing Mac testing. Remember to run a git pull to sync your local repo with the latest change in this branch.