Skip to content

Use Bordado in code, tests and docs#673

Open
santisoler wants to merge 19 commits intomainfrom
bordado
Open

Use Bordado in code, tests and docs#673
santisoler wants to merge 19 commits intomainfrom
bordado

Conversation

@santisoler
Copy link
Copy Markdown
Member

Add bordado as required dependency. Replace some of the Verde functions (like grid_coordinates, get_region, rolling_window, etc.) for their Bordado counterparts in source code, tests, and docs. Update the code that uses those functions: keep in mind that Bordado functions are n-dimensional, so additional coordinates are not ignored as in Verde ones.

Relevant issues/PRs:

Fixes #656

@santisoler santisoler marked this pull request as ready for review April 21, 2026 20:54
@santisoler santisoler requested a review from leouieda April 21, 2026 20:55
@santisoler santisoler added this to the v0.8.0 milestone Apr 21, 2026
Copy link
Copy Markdown
Member

@leouieda leouieda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Made some small suggestions. I also noticed that Bordado is missing from the list in https://www.fatiando.org/harmonica/dev/install.html#dependencies (Boule is too, actually).

# Define grid coordinates
region = vd.get_region(coordinates)
grid_coords = vd.grid_coordinates(
region = bd.get_region((easting, northing))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also do

Suggested change
region = bd.get_region((easting, northing))
region = bd.get_region(coordinates[:2])

but it's not much simpler. But just in case you don't want to use (easting, northing) all the time.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I think I prefer explicitly passing easting and northing in the guide. Since we are already defining those variables, it's not a big deal.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽

Comment thread src/harmonica/_equivalent_sources/cartesian.py Outdated
import verde.base as vdb
from numba import jit
from sklearn.utils.validation import check_is_fitted
from verde import BlockReduce, median_distance
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep the import verde as vd to maintain the clear namespace separation.


import numpy as np
import verde.base as vdb
from bordado import get_region, rolling_window
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd replace this with import bordado as bd to keep the namespace.

Comment thread src/harmonica/_forward/prism_layer.py Outdated
import numpy as np
import verde as vd
import xarray as xr
from verde import make_xarray_grid
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same about the import.

import numpy as np
import pooch
import verde as vd
from verde import make_xarray_grid
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use Bordado in the documentation for generating coordinates

2 participants