[JOSS review] Warnings and issues when running examples

nmstreethran commented 5 months ago

https://github.com/openjournals/joss-reviews/issues/6734

I managed to run the examples after installing the prerequisites and REHO from PyPI and changing the solver to highs.

Warnings

The following warnings appear when running the code. I think these are easy to fix so you should try to address them if possible.

/mnt/Backup/Downloads/REHO_pypi/.venv/lib/python3.11/site-packages/reho/model/preprocessing/QBuildings.py:91: SAWarning: Did not recognize type 'geometry' of column 'geometry' metadata.reflect(schema=self.db_schema)
/mnt/Backup/Downloads/REHO_pypi/.venv/lib/python3.11/site-packages/reho/model/preprocessing/QBuildings.py:436: FutureWarning: You are adding a column named 'geometry' to a GeoDataFrame constructed without an active geometry column. Currently, this automatically sets the active geometry column to 'geometry' but in the future that will no longer happen. Instead, either provide geometry to the GeoDataFrame constructor (GeoDataFrame(... geometry=GeoSeries()) or useset_geometry('geometry')to explicitly set the active geometry column. new_buildings_data[REHO_index] = df_buildings[key]
/mnt/Backup/Downloads/REHO_pypi/.venv/lib/python3.11/site-packages/reho/model/preprocessing/QBuildings.py:522: FutureWarning: You are adding a column named 'geometry' to a GeoDataFrame constructed without an active geometry column. Currently, this automatically sets the active geometry column to 'geometry' but in the future that will no longer happen. Instead, either provide geometry to the GeoDataFrame constructor (GeoDataFrame(... geometry=GeoSeries()) or useset_geometry('geometry')to explicitly set the active geometry column. new_roofs_data[REHO_index] = df_roofs[key]
/mnt/Backup/Downloads/REHO_pypi/.venv/lib/python3.11/site-packages/reho/model/postprocessing/building_scale_network_builder.py:103: RuntimeWarning: invalid value encountered in scalar divide df_aim = {'SC': (onsite_elec - df_exp * 1000) / onsite_elec,
/mnt/Backup/Downloads/REHO_pypi/.venv/lib/python3.11/site-packages/reho/model/preprocessing/clustering.py:243: FutureWarning: The frame.append method is deprecated and will be removed from pandas in a future version. Use pandas.concat instead. df = df.append(df_mod, ignore_index=True)

Issues

Data file

The data file used in the 3b_Custom_infrastructure.py example points to '../../reho/data/infrastructure/building_units.csv'. If the user does not clone the entire REHO repository and only downloads the scripts directory to run the examples after installing the package from PyPI, this particular example will fail to run. Could you copy that data file into the scripts/examples directory as well to prevent this? I also noticed that all the data files are included in the REHO PyPI package and are installed as part of the library in the venv.

Traceback (most recent call last):
  File "/mnt/Backup/Downloads/REHO_pypi/scripts/examples/3b_Custom_infrastructure.py", line 30, in <module>
    units = infrastructure.initialize_units(scenario, grids, building_data=path_to_custom_units)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/mnt/Backup/Downloads/REHO_pypi/.venv/lib/python3.11/site-packages/reho/model/infrastructure.py", line 466, in initialize_units
    building_units = prepare_units_array(building_data, exclude_units, grids)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/mnt/Backup/Downloads/REHO_pypi/.venv/lib/python3.11/site-packages/reho/model/infrastructure.py", line 374, in prepare_units_array
    unit_data = file_reader(file)
                ^^^^^^^^^^^^^^^^^
  File "/mnt/Backup/Downloads/REHO_pypi/.venv/lib/python3.11/site-packages/reho/paths.py", line 78, in file_reader
    file = Path(path_handler(file))
                ^^^^^^^^^^^^^^^^^^
  File "/mnt/Backup/Downloads/REHO_pypi/.venv/lib/python3.11/site-packages/reho/paths.py", line 73, in path_handler
    raise FileNotFoundError('The relative path that was given is not a valid file.')
FileNotFoundError: The relative path that was given is not a valid file.
Execution of 3b_Custom_infrastructure.py failed with error: Command '['python', '/mnt/Backup/Downloads/REHO_pypi/scripts/examples/3b_Custom_infrastructure.py']' returned non-zero exit status 1.

AMPL licence command

When running 4b_Sensitivity_analysis.py, there was an error caused by line 222 in reho/model/postprocessing/sensitivity_analysis.py, which is os.system('cmd /c "ampl_lic restart"'). This is a Windows-specific command which will cause an error on any other system. Could you please fix this to account for other operating systems?

DorsanL commented 3 months ago

All the mentioned issues have been addressed here: https://github.com/IPESE/REHO/commit/4a63b03aec4f85ae097582e4bd1ab2c3b5d03e0c

no more warnings regarding geometry, division by 0, or deprecated methods
I placed the data/ folder directly within scripts/examples/ to avoid downloading it separately
regarding the reho/data/ folder, it is indeed part of the package as it contains essential data for the model to run (as well as default data, which the user can replace by supplying specific files for weather, units and grids specifications)
the Windows-specific command cmd /c "ampl_lic restart" has been placed in an appropriate condition (this command is used when the model is run with distributed computing, for a very large number of optimizations)

nmstreethran commented 3 months ago

There's an error when running 4b_Sensitivity_analysis.py:

python 4b_Sensitivity_analysis.py 
Traceback (most recent call last):
  File "/media/nithiya/SSD2/REHO-git/scripts/examples/4b_Sensitivity_analysis.py", line 9, in <module>
    reader = QBuildingsReader()
             ^^^^^^^^^^^^^^^^
NameError: name 'QBuildingsReader' is not defined

DorsanL commented 2 months ago

Thank you for the notification, this has been solved now ✅ (https://github.com/IPESE/REHO/commit/3860931485cd3de224461d0d6f58c16d82d498af)

nmstreethran commented 2 months ago

Thanks for confirming!

IPESE / REHO