IMMM-SFA / tell

A model to predict Total ELectricity Loads (TELL)
https://immm-sfa.github.io/tell/
BSD 2-Clause "Simplified" License
24 stars 10 forks source link

[JOSS] Comments regarding documentation #36

Closed euronion closed 2 years ago

euronion commented 2 years ago

I have a couple of comments and suggestions after working through the documentation:

  1. Installation

tell is based on open-source publicly accessible data. (https://immm-sfa.github.io/tell/tell_quickstarter.html#install-the-package-of-data-underpinning-tell)

The term "open data" rather than "open source" is more appropriate, cf. e.g. this post

  1. API reference

From the API reference on the website I was unable to find information on tell.train as mentioned in the quickstarter notebook. Maybe one or more modules are not properly included in the API reference?

  1. Configuration

The quickstarter mentions default settings for MLP training as stored in the GitHub repository. I think these settings should be linked (to the respective file) or included in the documentation.

  1. Spatial resolution

For temporal resolution the documentation states hourly resolution. For spatial resolution, the user has to read quite far to find:

I suggest both aspects, as they are properties of major relevance to interested potential users, should be placed as prominently as the information on the hourly resolution.

I also suggest you highlight the aspect of geographical scope and resolution in the beginning of the JOSS paper.

  1. Target audience

Consider specififying the target audience and users of your package in a popular location like here

xref https://github.com/openjournals/joss-reviews/issues/4472

mcgrathc commented 2 years ago

Please see this PR for the changes:https://github.com/IMMM-SFA/tell/pull/41

Installation tell is based on open-source publicly accessible data. (https://immm-sfa.github.io/tell/tell_quickstarter.html#install-the-package-of-data-underpinning-tell) The term "open data" rather than "open source" is more appropriate, cf. e.g. this post: adopted

API reference From the API reference on the website I was unable to find information on tell.train as mentioned in the quickstarter notebook. Maybe one or more modules are not properly included in the API reference?: updated

Spatial resolution For temporal resolution the documentation states hourly resolution. For spatial resolution, the user has to read quite far to find: This package is only inteded for the CONUS region, a subset of USA What the adequate spatial resolution is ("Work at a spatial resolution adequate for input to a unit commitment/economic dispatch (UC/ED) model."). I suggest both aspects, as they are properties of major relevance to interested potential users, should be placed as prominently as the information on the hourly resolution.: added to quickstarter

I also suggest you highlight the aspect of geographical scope and resolution in the beginning of the JOSS paper.:The authors believe the first statement in the summary highlights both the geographical scope and resolution: The purpose of the Total ELectricity Load (tell) model is to generate 21st century profiles of hourly electricity load (demand) across the Conterminous United States (CONUS).

Target audience Consider specififying the target audience and users of your package in a popular location like here: added to index

comments on including MLP settings for user modification to follow

mcgrathc commented 2 years ago

Below addresses the remaining comments on including MLP settings for user modification:

Comment: The quickstarter mentions default settings for MLP training as stored in the GitHub repository. I think these settings should be linked (to the respective file) or included in the documentation. Consider linking the file or including the file contents in the documentation. It might be potentially interesting for users to look up and change these settings in their local setup; especially if these settings might change between versions. Consider describing the local availability of the settings file in the Python installation location ( tell.file folder) so show the user how to check the values used by their version. Response: We thank the reviewer for making this suggestion. We’ve modified the MLP section of the tell User Guide to describe where the default settings for the MLP models are stored in the tell repository. We also explicitly link to the mlp_settings.yml file in the User Guide. “Details of the MLP predictive variables are included in the table below. The default parameter settings for training the MLP models are stored in the mlp_settings.yml <https://github.com/IMMM-SFA/tell/blob/main/tell/data/mlp_settings.yml>_ file in /data folder in the tell repository. The hyperparameters for the tell MLP models (e.g., hidden layer sizes, maximum iterations, and validation fraction) were determined using a grid search approach. Hyperparameters were allowed to vary across BAs. Default hyperparameters for each BA are also included in the /data/models folder in the tell repository.”

mcgrathc commented 2 years ago

Closing if no further comments. Will reopen if needed.