[JOSS Review] Installation and documentation

[nmstreethran]

Hi @AlexanderJuestel, I'm reviewing pyheatdemand for JOSS at openjournals/joss-reviews#6275. This issue concerns the installation instructions and documentation of your package. Please let me know if anything is unclear. I haven't finished going through the heat demand examples and the paper yet; I'll open more issues for these if necessary.

Documentation

• For consistency, you could structure the installation instructions in the README to match that of the readthedocs documentation, i.e. split Pip and Conda installation instructions as subsections.
• Feature the documentation more prominently in the README. Right now, there's a badge for the docs build but a section with a link to the documentation on readthedocs will be useful. You can maybe merge the API reference / local documentation build instructions here.
• Add more details on installing from source, i.e. using environment_dev.yml and requirements.txt after cloning the repository.
• Add a link to the contributing guidelines to the README.
• Although you have included instructions for continuous integration, I think you should add the testing instructions to the documentation, perhaps under the installation instructions, for testing locally.
• pytest-cov isn't included in environment_dev.yml and requirements.txt, but I think it should be for the purpose of testing locally.
• I also noted that the packages in environment_dev.yml and requirements.txt do not match. Perhaps you could include the missing sphinx packages in requirements.txt for consistency?

I've included my installation steps below for your reference, and also in case I'm doing something wrong.

Installation with Pip

I suggest updating the Pip install command to python -m pip install pyheatdemand, which is recommended (see https://pip.pypa.io/en/stable/user_guide/#installing-packages) (and maybe also use a virtual environment).

python -m venv .env
source .env/bin/activate
python -m pip install pyheatdemand

Installation from source (including testing; I added pytest-cov to requirements.txt):

git clone https://github.com/AlexanderJuestel/pyheatdemand.git
cd pyheatdemand
python -m venv .env
source .env/bin/activate
python -m pip install -r requirements.txt
cd test
pytest --cov

Installation with Conda

Is there a reason why environment.yml and environment_dev.yml use Pip to install some dependencies when they already exist in conda-forge? Installing from environment.yml failed for me with the following output:

Channels:
 - conda-forge
 - defaults
Platform: linux-64
Collecting package metadata (repodata.json): done
Solving environment: done


==> WARNING: A newer version of conda exists. <==
    current version: 23.11.0
    latest version: 24.1.0

Please update conda by running

    $ conda update -n base -c conda-forge conda



Downloading and Extracting Packages:
                                                                                                                                                                              
Preparing transaction: done                                                                                                                                                   
Verifying transaction: done                                                                                                                                                   
Executing transaction: done                                                                                                                                                   
Installing pip dependencies: \ Ran pip subprocess with arguments:                                                                                                             
['/home/nms/miniconda3/envs/pyheatdemand/bin/python', '-m', 'pip', 'install', '-U', '-r', '/run/media/nms/Backup/Downloads/pyheatdemand/condaenv._ufstax7.requirements.txt', '--exists-action=b']                                                                                                                                                           
Pip subprocess output:                                                                                                                                                        
Collecting p (from -r /run/media/nms/Backup/Downloads/pyheatdemand/condaenv._ufstax7.requirements.txt (line 1))                                                               
  Using cached p-1.4.0-py3-none-any.whl (10 kB)                                                                                                                               
                                                                                                                                                                              
Pip subprocess error:
ERROR: Could not find a version that satisfies the requirement y (from versions: none)
ERROR: No matching distribution found for y

failed

CondaEnvException: Pip failed

I updated the environment files to use packages from conda-forge only as detailed below, which fixed this issue.

Updated environment.yml

I updated environment.yml to the following (I didn't think the other dependencies are needed as this is not the development version):

name: pyheatdemand
channels:
  - conda-forge
dependencies:
  - python>=3.10
  - pyheatdemand

... and then installed the packages as follows, which proceeded without issues:

git clone https://github.com/AlexanderJuestel/pyheatdemand.git
cd pyheatdemand
conda env create
conda activate pyheatdemand

Updated environment_dev.yml

For the development version, I updated environment_dev.yml as follows (I changed the underscores to dashes for the sphinx packages and added pytest-cov):

name: pyheatdemand
channels:
  - conda-forge
dependencies:
  - python>=3.10
  - geopandas
  - rasterstats # also installing rasterio
  - matplotlib
  - tqdm
  - geopy
  - osmnx
  - sphinx-book-theme==0.3.3
  - sphinx-copybutton
  - nbsphinx
  - pytest-cov

... then installed and tested the packages with no issues:

git clone https://github.com/AlexanderJuestel/pyheatdemand.git
cd pyheatdemand
conda env create -f environment_dev.yml
conda activate pyheatdemand
cd test
pytest --cov

Tests

The output of pytest --cov shows 11 warnings including deprecation and CRS mismatch warnings which should ideally be addressed:

============================================================================ test session starts =============================================================================
platform linux -- Python 3.12.1, pytest-8.0.0, pluggy-1.4.0
rootdir: /run/media/nms/Backup/Downloads/pyheatdemand/test
plugins: cov-4.1.0
collected 38 items                                                                                                                                                           

test_processing.py ......................................                                                                                                              [100%]

============================================================================== warnings summary ==============================================================================
../../../../../../../home/nms/miniconda3/envs/pyheatdemand-dev/lib/python3.12/site-packages/dateutil/tz/tz.py:37
  /home/nms/miniconda3/envs/pyheatdemand-dev/lib/python3.12/site-packages/dateutil/tz/tz.py:37: DeprecationWarning: datetime.datetime.utcfromtimestamp() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.datetime.fromtimestamp(timestamp, datetime.UTC).
    EPOCH = datetime.datetime.utcfromtimestamp(0)

test_processing.py:3
  /run/media/nms/Backup/Downloads/pyheatdemand/test/test_processing.py:3: DeprecationWarning: 
  Pyarrow will become a required dependency of pandas in the next major release of pandas (pandas 3.0),
  (to allow more performant data types, such as the Arrow string type, and better interoperability with other libraries)
  but was not found to be installed on your system.
  If this would cause problems for you,
  please provide us feedback at https://github.com/pandas-dev/pandas/issues/54466
          
    import pandas as pd

test_processing.py::test_get_building_footprints
  /home/nms/miniconda3/envs/pyheatdemand-dev/lib/python3.12/site-packages/pyproj/transformer.py:820: DeprecationWarning: Conversion of an array with ndim > 0 to a scalar is deprecated, and will error in future. Ensure you extract a single element from your array before performing this operation. (Deprecated NumPy 1.25.)
    return self._transformer._transform_point(

test_processing.py::test_calculate_hd_sindex[hd_gdf0-mask_gdf0]
test_processing.py::test_calculate_hd_sindex[hd_gdf0-mask_gdf0]
test_processing.py::test_calculate_hd_sindex[hd_gdf0-mask_gdf0]
test_processing.py::test_calculate_hd_sindex_points[hd_gdf0-mask_gdf0]
test_processing.py::test_calculate_hd_sindex_points[hd_gdf0-mask_gdf0]
test_processing.py::test_calculate_hd_sindex_lines[hd_gdf0-mask_gdf0]
test_processing.py::test_calculate_hd_sindex_lines[hd_gdf0-mask_gdf0]
  /home/nms/miniconda3/envs/pyheatdemand-dev/lib/python3.12/site-packages/geopandas/geodataframe.py:1525: SettingWithCopyWarning: 
  A value is trying to be set on a copy of a slice from a DataFrame.
  Try using .loc[row_indexer,col_indexer] = value instead
  
  See the caveats in the documentation: https://pandas.pydata.org/pandas-docs/stable/user_guide/indexing.html#returning-a-view-versus-a-copy
    super().__setitem__(key, value)

test_processing.py::test_calculate_hd_street_segments[gdf_roads0-gdf_buildings0]
  /run/media/nms/Backup/Downloads/pyheatdemand/pyheatdemand/processing.py:1448: UserWarning: CRS mismatch between the CRS of left geometries and the CRS of right geometries.
  Use `to_crs()` to reproject one of the input geometries to match the CRS of the other.
  
  Left CRS: EPSG:25832
  Right CRS: None
  
    gdf_joined = gpd.sjoin_nearest(gdf_buildings,

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

---------- coverage: platform linux, python 3.12.1-final-0 -----------
Name                                                                      Stmts   Miss  Cover
---------------------------------------------------------------------------------------------
/run/media/nms/Backup/Downloads/pyheatdemand/pyheatdemand/__init__.py         5      0   100%
/run/media/nms/Backup/Downloads/pyheatdemand/pyheatdemand/processing.py     321      0   100%
__init__.py                                                                   0      0   100%
test_processing.py                                                          412      0   100%
---------------------------------------------------------------------------------------------
TOTAL                                                                       738      0   100%

====================================================================== 38 passed, 11 warnings in 26.68s ======================================================================

[AlexanderJuestel]

@nmstreethran, wow thank you for this thorough review. I will go through your remarks and implement them in the next coming days!

[AlexanderJuestel]

Documentation

• For consistency, you could structure the installation instructions in the README to match that of the readthedocs documentation, i.e. split Pip and Conda installation instructions as subsections.

Docs and Readme were changed in 5e0462c

• Feature the documentation more prominently in the README. Right now, there's a badge for the docs build but a section with a link to the documentation on readthedocs will be useful. You can maybe merge the API reference / local documentation build instructions here.

A reference to the documentation page was added to the readme in a5d31f6. I would leave the API Reference reference at the end of the documentation.

• Add more details on installing from source, i.e. using environment_dev.yml and requirements.txt after cloning the repository.

I added a little more information on installing from source to the Readme in c372ade

• Add a link to the contributing guidelines to the README.

The contribution guidelines were added to the readme in 14c4058

• Although you have included instructions for continuous integration, I think you should add the testing instructions to the documentation, perhaps under the installation instructions, for testing locally.

I added the paragraph about the CI to the installation docs in 4baa0a6

• pytest-cov isn't included in environment_dev.yml and requirements.txt, but I think it should be for the purpose of testing locally.

I updated the installation files according to your comments in 17abe8a

• I also noted that the packages in environment_dev.yml and requirements.txt do not match. Perhaps you could include the missing sphinx packages in requirements.txt for consistency?

Sphinx was added to the requirements in 3064028

Tests

There was indeed the CRS missing on one dataset. This is now set within the test environment and matched with the other GeoDataFrames CRS. It was fixed in 73f41fd.

The deprecation warning is issued on the GeoPandas site. So nothing I can do about it, unfortunately.

[AlexanderJuestel]

@nmstreethran, please see my edits in the linked commits above. I should have resolved all your requested changes. Thanks again for this thorough review so far.

[nmstreethran]

@AlexanderJuestel That's great, thank you for addressing my comments. I'll get back to you tomorrow with the rest of my comments on the paper and examples.