search

modflowpy / flopy

A Python package to create, run, and post-process MODFLOW-based models.
https://flopy.readthedocs.io
Other
506 stars 306 forks source link

bug: failing test_gridintersect.py #2214

Closed martclanor closed 2 months ago

martclanor commented 2 months ago

Hello devs! First of all, I'm a bit new here and I'm not really sure if I should put this up as an issue or discussion question but here it is anyway.

To Reproduce Steps to reproduce the behavior:

When I run all the tests with pytest -v -n auto, I get this error:

FAILED test_gridintersect.py::test_linestring_offset_rot_structured_grid - AssertionError: assert 3 == 2

Iiuc, basically, this is checking how many cells are intersected by a horizontal line on the center of this image:

horizontal_line = LineString([(5, 10.0 + np.sqrt(200.0)), (15, 10.0 + np.sqrt(200.0))])

image

The computed value on my test is 3, probably because the horizontal line's y-position is approximated to some extent and thus cannot be put exactly at the middle.

The failing line used to be an assertion statement that allows the value to either be 2 or 3 (instead of just 2) so it seems to be a deliberate change (see https://github.com/modflowpy/flopy/commit/40c03913bdf4329e8540d365ccfffe1c2bf4fccf#diff-6ab39ba678301b37903b1a7fe96e20267cba1b3bfeaf2977333b5774ba699bb7L1217) and I'm just not sure what the reasoning here was.

Laptop:

dbrakenhoff commented 2 months ago

Hi @martclanor, thanks for posting this issue! I'll look into this. Like you said, its probably a floating point issue, and one that seemed to be solved (hence the change in assertion value). I'll post back here when I have some news.

mwtoews commented 2 months ago

I've found this one too, and the cause is slightly different values from built-in sin versus NumPy sin (within the shapely.affinity module). Not sure why they are different recently.

A good solution is to modify this test to avoid the small intersection, like change the rotation angle to definitely intersect three cells.

martclanor commented 2 months ago

Ah I see, thanks for clearing that out @mwtoews and suggesting a solution.

@dbrakenhoff , since it's apparently a simple fix, I'll just submit a quick PR on this, can you maybe do the review? Thanks in advance!

wpbonelli commented 2 months ago

closed by #2216