-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add gradient-boosted equivalent sources #275
Conversation
Rename the source file to gradient_boosted.py. Enable the block_size argument after the last merge.
The residue array is initialized within the private function, there's no need to do it outside of it since we won't support a warm run.
Add an option to the _create_windows method to disable shuffling. This is intended to be used only for testing purposes, not IRL.
There's no need this method cannot be a separate private function.
There's no need to prevent overwriting the Jacobian matrix after the fitting process since the update of the residuals are done through a sum because we need to compute the predictions on every observation point, not only over those inside the window.
Use the South Africa gravity dataset.
Ditch the assert_mse function from tests/utils.py. Replace the large region test function for a one that compares the predictions against the data and a denser grid, but on the same small region than other tests are using. The comparison is carried out with a npt.assert_allclose but setting atol instead of rtol.
self.coefs_[point_window] += coeffs_chunk | ||
self.errors_ = np.array(errors) | ||
|
||
def _create_windows(self, coordinates, shuffle_windows=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the shuffle_windows
argument to this function in order to run a test that depends on not shuffling (the one that checks that the correspondence between sources windows and data windows). I'm not planning to make it public since it's a bad idea not to shuffle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes perfect sense and it says so in the docstring which is perfect. How about just shuffle
since the function is already _create_windows
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used shuffle
at first, but the linter warned me that shuffle
was taken by the import of the sklearn.utils.shuffle
function. I agree with renaming the argument, so I would need to import the function with an alias.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better than that would be import the module instead from … import
. That’s always preferred to importing single functions since it keeps the namespaces separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Can't wait to use it 🙂 Left a few suggestions but feel free to reject them if your disagree. This is good to merge 👍
self.coefs_[point_window] += coeffs_chunk | ||
self.errors_ = np.array(errors) | ||
|
||
def _create_windows(self, coordinates, shuffle_windows=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes perfect sense and it says so in the docstring which is perfect. How about just shuffle
since the function is already _create_windows
?
Co-authored-by: Leonardo Uieda <[email protected]>
By making it a regular method we avoid the need to pass the same arguments twice.
Use an alias for the sklearn.utils.shuffle function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace "magnetic" for "gravity" in the example
Add a new
EquivalentSourcesGB
class that implements gradient-boostingequivalent sources. It's a subclass of
EquivalentSources
, takes newwindow_size
andrandom_state
arguments and has a modifiedfit
method thatapplies the gradient-boosting algorithm. The amount of overlapping is fixed to
50%. The windows are randomly shuffled by default. Add new test functions for
the new class and its features. Add a gallery example for gradient-boosted
equivalent sources.
Fixes #252
Reminders:
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
and the base__init__.py
file for the package.AUTHORS.md
file (if you haven't already) in case you'd like to be listed as an author on the Zenodo archive of the next release.