Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

review for joss #10

Closed
ibarraespinosa opened this issue Dec 31, 2023 · 12 comments
Closed

review for joss #10

ibarraespinosa opened this issue Dec 31, 2023 · 12 comments

Comments

@ibarraespinosa
Copy link
Contributor

Hi

I'm reviewing for JOSS here openjournals/joss-reviews#6033

Im using the configuration show in the picture (android), but may use other tools Screenshot_20231231-121149.png

@ibarraespinosa
Copy link
Contributor Author

Overview:

I'm running all examples sucessfully.
This package reads surface temperature from atmospheric transport models in NetCDF format.
The author was able to create a function to read NetCDF relaying on raster.
The author preferred wide format to process data.frames, dor instance:

# example data: WRF hourly climate at 12-km resolution
path_to_climate_ncdf <- helios::pkg_example('wrfout_d01_2020-01-01_01%3A00%3A00_sub.nc')

temperature <- helios::read_ncdf(ncdf = path_to_climate_ncdf,
                                 model = 'wrf',
                                 var = 'T2',
                                 time_periods = 2020)

The objective is to calculate temperatures outside comfort. The (figure)[https://jgcri.github.io/helios/articles/vignetteFigs/helios_gcam-usa_workflow.jpg] should be in Celsius or Fahrenheit, but calculations in kelvin.
The author may include units::set_units() to control conversions of temperature units
The code in helios::hdcd looks sound. I'm not a fan of dplyr, I prefer data.table, but code looks ok.
The documentation is (incomplete)[https://jgcri.github.io/helios/reference/hdcd.html]. For instance, the argument reference_temp_F is 'Default = 65' which is not explanatory. Does that mean that the temperature inputted must be in Fahrenheit?
Global Change Assessment Model (GCAM) should be defined in the README (first page)
In general, everything looks fine

@ibarraespinosa
Copy link
Contributor Author

A link to (CONTRIBUTING)[https://jgcri.github.io/helios/CONTRIBUTING.html] should be added in README

@ibarraespinosa
Copy link
Contributor Author

Compile the paper https://github.com/JGCRI/helios/blob/main/paper/paper.md (use rticles) and upload the pdf in the repo

@ibarraespinosa
Copy link
Contributor Author

The caption of Figure 1 is too limited. Should expand it

@ibarraespinosa
Copy link
Contributor Author

Can you change units of figure 1 from kelvin to Celsius or F?

@ibarraespinosa
Copy link
Contributor Author

ibarraespinosa commented Jan 1, 2024

Can you add this section into the manuscript?

  • State of the field: Do the authors describe how this software compares to other commonly-used packages?

@mengqi-z
Copy link
Collaborator

mengqi-z commented Jan 9, 2024

Hi @ibarraespinosa,

Thank you for your valuable comments and suggestions. We have addressed them, and you can review all the commits summarized in the pull request #12.

Please note that we didn't change the temperature unit from "K" to "Celsius" or "Fahrenheit" in the workflow figure because Kelvin is the unit of the raw temperature data for both CMIP and WRF cases. After reading the raw temperature data, helios will convert the temperature from Kelvin to Fahrenheit before calculating heating and cooling degrees.

We appreciate the reviewer's suggestion of using units::set_units() to clarify the output data units. However, we believed that having the units directly associated with the values could make later calculations a bit challenging for users who wish to manipulate the outputs. Instead, we made the following updates:

  • We added an attribute to the output table from helios::read_ncdf to indicate the unit of the raw temperature data. Users can check the unit information using str(output) or attr(output, 'metadata').

  • We also updated the unit column in the output table from helios::hdcd to indicate that the degree-days/hours are Fahrenheit-based.

Thank you once again for your review of helios. Please don't hesitate to let us know if you have any further comments or questions.

@ibarraespinosa
Copy link
Contributor Author

Thanks for the change in the code, but the documentation has to improve as well. here, please include a better documentation for the argument. reference_temp_F

@mengqi-z
Copy link
Collaborator

Hi @ibarraespinosa, Thank you for catching that. I forgot to document the package before pushing to Github. Now all the arguments documentation should be updated accordingly.

@ibarraespinosa
Copy link
Contributor Author

I figured.
@mengqi-z nice,
the package looks fine. I will a second review to the code and documentation to then focus more on the manuscript. Once I'm done with the review, I will focus on GCAMREPORT

@mengqi-z
Copy link
Collaborator

Sounds good. Thank you, @ibarraespinosa !

@ibarraespinosa
Copy link
Contributor Author

your package looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants