ec-jrc / pyPoseidon

Framework for Hydrodynamic simulations
https://pyposeidon.readthedocs.io/
European Union Public License 1.2
20 stars 8 forks source link

Introduce boundary module #29

Closed brey closed 2 years ago

brey commented 3 years ago

The tests run locally. I don't know why one of them doesn't work on Actions. It has to do with the new test data item i.e. folder bl.zip.

brey commented 3 years ago

On my fork the conda ones were failing too. Here only the venv. Weird.

pmav99 commented 3 years ago

Could you try to rename the bl.zip from your local PC so that it gets re-downloaded. This way we will see if the uploaded data are the correct ones.

brey commented 3 years ago

I have deleted the whole folder before running the tests locally. They work. Just did it again. Apparently they worked for conda too here (but not on my fork). This is very weird.

pmav99 commented 3 years ago

Do you remember when we discussed that an else branch should always be added to if/elif/elif statements?Well, here we don't really follow that: https://github.com/brey/pyPoseidon-dev/blob/b3e46fc26033927ea0a5d8474a1c463cf4951416/pyposeidon/boundary.py#L116-L125

I don't know why the tests pass on your Mac, but the problem is that the geometry attribute is not an instance of dict, str or GeoDataFrame. Therefore the df object is never created and, as expected, the right hand side of line 125 which uses df throws an UnboundLocalError which means that the name has not been defined

The reason why this happens is the logic here: https://github.com/brey/pyPoseidon-dev/blob/b3e46fc26033927ea0a5d8474a1c463cf4951416/pyposeidon/boundary.py#L73-L103

In a nutshell, the gp.GeoDataFrame.from_file(geometry) fails. This can also be understood since the geometry is not a file, trying with geopandas Dataset message is printed in the tests output.

So there are two issues here:

  1. The zip file cannot be parsed by the version of geopandas we are using.
  2. The code that handles what happens when the parsing fails is not really robust.
brey commented 3 years ago

You are right about the flow control but we knew the problem was in the read_file. I will add a graceful exit if the read fails. The issue however is the geopandas version. Reading zip files was introduced in 0.9.0 (https://geopandas.org/docs/changelog.html). So we need to update poetry. I went back to my fork and there the conda failed due to dropped connection with dropbox. So I will take care of it.

pmav99 commented 3 years ago

BTW, according to the docs geopandas.read_file() should be preferred over GeoDataFrame.from_file(), so please change that, too.

Other than that, it should be possible to handle the zip file in older versions, too. To do so we should need something like this (not tested, so it might need adjustments):

import zipfile

try:
    with zipfile.ZipFile(self.geometry, "r") as my_zip:
        gp.read_file(my_zip)
except BadZipFile:
    # this is not a zip file, move on with the next method or handle the exception in whatever way is appropriate
    # if it is a zip file and geopandas/fiona fails to parse it, 
    # then a Fiona-related exception will be raised by `read_file()`
    # but we don't handle it, so it will be raised anyway
    ...
brey commented 3 years ago

There is no need to add another import for now. Anyway I cleaned it up a bit and now the tests pass. The work is not done. So we can pull it in now and do another one later or let it hanging until the workflow and tests are complete.

brey commented 2 years ago

@pmav99 Can you merge this one. The next will be more targeted.