Open yifatdzigan opened 3 years ago
Hi @rogerkuou, looks great!
I have run the tests, the notebook and the python scripts, it is nicely commented so it has been easy to follow the workflows. The package seems to me very well structured, and it seems easy to expand the NN models implemented.
Just let you here all my comments, but overall very nice work!
setup.py
there is a comma missing after geopandas
in the dependency list, making installation with pip
fail. I have also noticed that the dependencies are listed both in the setup.py
file and in the requirements.txt
. Maybe it would be nice to keep dependencies in only one place, so that one needs to update a single list whenever a new package needs to be added to the dependencies?conda
, then install the repo via pip - is this an issue for other OSs as well? If this is the case, maybe some extra details should be mentioned in the README? .pickle
to make clear the format? pandas.Dataframe
s. I think one could also work with geopandas
’s GeoDataFrame
s, making explicit use of the lat/lon information that these data frames include. These could then be saved e.g. in GeoJSON format (which is also human-readable) instead of pickle binaries, even though maybe the latter is less space consuming. Using GeoDataFrame
s would make some of the geo-tasks easier, e.g. the intersection with shape files. imshow
for matrix/raster representations, which might be particularly suitable if the data always lays on a regular grid. Merged all minor fixes to master. The initial release is available at: https://github.com/VegeWaterDynamics/ml_lsmodel_ascat
Including documentation, code refinement and unit tests