Skip to content
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 conditional to set _mv variable to np.uint64 data type #47

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions CONTRIBUTING.rst
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,13 @@ First, clone pyflwdir's ``git`` repo and navigate into the repository:
$ git clone [email protected]:Deltares/pyflwdir.git
$ cd pyflwdir

Then, make and activate a new pyflwdir conda environment based on the environment.yml
Then, make and activate a new pyflwdir conda environment based on the `environment.yml`
file contained in the repository:

.. code-block:: console

$ conda env create -f envs/pyflwdir-dev.yml
$ conda activate pyflwdir-dev
$ conda env create -f environment.yml
$ conda activate pyflwdir

Finally, build and install PyFlwDir:

Expand Down
4 changes: 3 additions & 1 deletion pyflwdir/flwdir.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,12 @@ def __init__(
self.idxs_outlet = idxs_outlet
self._seq = idxs_seq
self._nnodes = nnodes
# either -1 for int or 4294967295 for uint32
# either -1 for int, 4294967295 for uint32, or 18446744073709551615 for uint64
self._mv = core._mv
if idxs_ds.dtype == np.uint32:
self._mv = np.uint32(self._mv)
if idxs_ds.dtype == np.uint64:

Choose a reason for hiding this comment

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

Just appears to be a dtype limitation?

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@fernando-aristizabal not exactly. The dtype assigned to self._mv needs to be the same as the dtype for idxs.ds. Those two dtypes need to line up, otherwise there were errors in numba code. In the downstream code, incorrect aliasing was occurring based on _mv's dtype. This was causing a mis-match in values between _mv and items in the idxs.ds array, which ultimately led to the IndexErrors being thrown.

In order to avoid the IndexErrors, for the large array indices case, where the idxs.ds.dtype is set to uint64, the dtype for _mv needs to be set to uint64 as well.

self._mv = np.uint64(self._mv)

# set placeholders only used if cache if True
self.cache = cache
Expand Down
31 changes: 29 additions & 2 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,35 @@ def test_data1(flwdir1_idxs, flwdir1_rank):


@pytest.fixture(scope="session")
def test_data(test_data0, flwdir0, test_data1, flwdir1):
return [(test_data0, flwdir0), (test_data1, flwdir1)]
def flwdir2():
np.random.seed(2345)
return from_dem(np.random.rand(15, 10)).to_array("d8")


@pytest.fixture(scope="session")
def flwdir2_idxs(flwdir2):
idxs_ds2, idxs_pit2, _ = core_d8.from_array(flwdir2, dtype=np.uint64)
return idxs_ds2, idxs_pit2


@pytest.fixture(scope="session")
def flwdir2_rank(flwdir2_idxs):
idxs_ds2, _ = flwdir2_idxs
rank2, n2 = core.rank(idxs_ds2, mv=np.uint64(core._mv))
seq2 = np.argsort(rank2)[-n2:]
return rank2, n2, seq2


@pytest.fixture(scope="session")
def test_data2(flwdir2_idxs, flwdir2_rank):
rank2, _, seq2 = flwdir2_rank
idxs_ds2, idxs_pit2 = flwdir2_idxs
return idxs_ds2, idxs_pit2, seq2, rank2, np.uint64(core._mv)


@pytest.fixture(scope="session")
def test_data(test_data0, flwdir0, test_data1, flwdir1, test_data2, flwdir2):
return [(test_data0, flwdir0), (test_data1, flwdir1), (test_data2, flwdir2)]


@pytest.fixture(scope="session")
Expand Down
3 changes: 2 additions & 1 deletion tests/test_pyflwdir.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ def test_flwdirraster_errors(flwdir0, flwdir0_idxs):


@pytest.mark.parametrize(
"test_data, flwdir", [("test_data0", "flwdir0"), ("test_data1", "flwdir1")]
"test_data, flwdir",
[("test_data0", "flwdir0"), ("test_data1", "flwdir1"), ("test_data2", "flwdir2")],
)
def test_flwdirraster_attrs(test_data, flwdir, request):
d8 = request.getfixturevalue(flwdir)
Expand Down
Loading