Closed editorialbot closed 9 months ago
editorialbot
commented
1 year ago Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.
For a list of things I can do to help you, just type:
@editorialbot commandsFor example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdfSoftware report:
github.com/AlDanial/cloc v 1.88 T=0.35 s (107.8 files/s, 12201.7 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
R 29 538 794 1804
Markdown 3 164 0 413
Rmd 2 160 197 104
TeX 1 9 0 91
YAML 1 2 4 19
JSON 2 0 0 2
-------------------------------------------------------------------------------
SUM: 38 873 995 2433
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repositoryWordcount for paper.md is 2359
editorialbot
commented
1 year ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1016/j.eiar.2015.10.004 is OK
- 10.1080/17512549.2015.1043643 is OK
- 10.1175/bams-d-11-00019.1 is OK
- 10.1016/j.buildenv.2021.107791 is OK
- 10.3390/land11050747 is OK
- 10.1111/j.1467-8306.1965.tb00529.x is OK
MISSING DOIs
- 10.1016/0013-9351(72)90023-0 may be a valid DOI for title: Some effects of the urban structure on heat mortality
- 10.21105/joss.03541 may be a valid DOI for title: GeoClimate: a Geospatial processing toolbox for environmental and climate studies
- 10.1016/j.landurbplan.2017.08.009 may be a valid DOI for title: Evaluating urban heat island in the critical local climate zones of an Indian city
INVALID DOIs
- None:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
martinfleis
commented
1 year ago 👋🏼 @MGousseff, @matthiasdemuzere, @wcjochem this is the review thread for the paper. All of our communications will happen here from now on.
All reviewers should create checklists with the JOSS requirements using the command @editorialbot generate my checklist. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.
The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues (and small pull requests if needed) on the software repository. When doing so, please mention https://github.com/openjournals/joss-reviews/issues/5445 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.
We aim for reviews to be completed within about 2-4 weeks, feel free to start whenever it works for you. Please let me know if any of you require significantly more time. We can also use editorialbot to set automatic reminders if you know you'll be away for a known period of time.
Please feel free to ping me (@martinfleis) if you have any questions/concerns.
Thanks!
martinfleis
commented
1 year ago @MGousseff please check the DOI suggestions above. If they are correct, include the DOIs in the paper. Thanks!
MGousseff
commented
1 year ago I'm sorry, I think the DOIs were in the bibtex, but flagged with url = in lieu of DOI =, I fix it right now and send the pull request asap.
MGousseff
commented
1 year ago Th DOIS have been fixed.
martinfleis
commented
1 year ago @editorialbot check references
editorialbot
commented
1 year ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1016/j.eiar.2015.10.004 is OK
- 10.1016/0013-9351(72)90023-0 is OK
- 10.1080/17512549.2015.1043643 is OK
- 10.1175/bams-d-11-00019.1 is OK
- 10.1016/j.buildenv.2021.107791 is OK
- 10.21105/joss.03541 is OK
- 10.3390/land11050747 is OK
- 10.1016/j.landurbplan.2017.08.009 is OK
- 10.1111/j.1467-8306.1965.tb00529.x is OK
MISSING DOIs
- None
INVALID DOIs
- None@editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
wcjochem
commented
1 year ago 
matthiasdemuzere
commented
1 year ago matthiasdemuzere
commented
1 year ago
- License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
@MGousseff: the repo contains a license file in markdown, whilst this checklist asks for a plain-text file? I am new to these JOSS requirements, so I ma not sure if this is fine @martinfleis?
MGousseff
commented
1 year ago Hello @matthiasdemuzere , thank you for getting involved. I followed the recommandations of Hadley Wickham in https://r-pkgs.org/ but for common licence, the licence file is even ignored (specified in the .Rbuildignor) as CRAN considers it as redudant with the licence specified in the DESCRIPTION file. So if a copy in full text format is needed, I think I can add it and add it's path in the .Rbuildignore, just let me know if it is needed.
matthiasdemuzere
commented
1 year ago
- Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
The lczexplore R package presents a tool “to compare different LCZ classifications”, or, more general, “any type of classifications on geographical units”.
Personally, I am not convinced that the current state of the package represents a substantial scholarly effort. I assume the code has been developed in the context of Bernard et al. (2023), a paper that is currently under review? Serving a specific purpose within this paper? Data from this paper is therefore also used as a sample dataset within this package.
But, I am not convinced this package will be useful for a broader audience, who might have very different use cases or types of spatial classifications? Other reasons for this concern::
Need: I’ve been working with LCZ maps for many years now. Yet I am not convinced the community is really interested in a tool that can compare two LCZ maps? Once you know about the difference / agreements between two maps, what then? How will you use this information? Without a proper reference (ground truth), I don’t really see what are the next steps one can do to put this to use? Bottom line: I am not convinced by the statement of need.
State of the field: I am a bit surprised by the shallow description here? The interest in LCZs is immense, yet this is not reflected here? Eg. the LCZ generator (Demuzere et al., 2021) that has received 5000+ LCZ submission in the past two years, the global LCZ map (Demuzere et al., 2022), or the many LCZ review papers, with Huang et al. (2023) likely being the most recent and comprehensive one?
Representation: From a LCZ content point of view, I wonder whether it really makes sense to compare maps that are developed from earth observation information and VGI/GIS layers? Typically their spatial units are very different, one being more representative for the coarser neighborhood-scale (as intended by Stewart and Oke (2012)), and one more on the block level? Please note that this comment might be more about semantics, and not a key reason for me to doubt the applicability of the package as such.
Coding standards / tests: the code seems relatively young, with most commits in the past 2 months only. As far as I can see, it has also not been tested outside the scope of the Bernard et al. (2023) paper, using e.g. different types of data? Even testing with raster LCZ maps from the most widely used sources such as the LCZ Generator or other continental or global maps seems limited? In any case, it does not seem very straightforward to me to do so, as exemplified by the Main Functions description in the README.md: The following functions are the core of this package : importLCZgen : imports the LCZ layer from a GIS (tested with geojson and shapefile files) importLCZwudapt : imports LCZ from the wudapt Europe Tiff. You'll have to use importLCZgen first to create the Bounding box of your zone of interest showLCZ : plots the map of your LCZ compareLCZ : compares two LCZ classifications of the same areas, output plots and data if this comparison confidSensib : explores how the agreement between two LCZ varies according to a confidence indicator associated for the LCZ value of each geom (sensibility analysis). Somehow the functionality seems to be there, but rather inaccessible, and definitely not in a shape I would expect from a package? To add, I eg. also do not see any tests to verify the functionality of this software?
In summary, I support the fact that the authors want to make this code publicly available. Yet in its current state, it feels more like a personal R library serving a specific goal than a tool that will be sufficiently useful for a broader community and more general applications? As such, I don’t think it is ready to be published in JOSS @martinfleis
MGousseff
commented
1 year ago Thank you @matthiasdemuzere for your review.
I will answer the points you have raised and propose some modifications.
About the Need: This is my main concern about your review. The fact that defining "ground truth" in LCZ studies is difficult is precisely why one needs a tool to compare LCZ classifications. As you noticed, the tool was developed for the need of the Bernard et al paper analysis to compare vectorial dataset but it has been directly thought as a generic method to compare any LCZ classification maps (thus raster is included). Maybe this was not clear in the manuscript. This part will be detailed in the future paper version. For example a function is available to load and process the WUDAPT Europe tiff map.
Concerning the need of the tool, we are a bit surprise about the comments “Yet I am not convinced the community is really interested in a tool that can compare two LCZ maps” and “I am not convinced by the statement of need.” Indeed even if WUDAPT is the main standard method and endorsed by the community, there are others LCZ sources ( ground truth, GIS approaches, machine learning...). So there is a crucial need to objectively quantify differences obtained between two LCZ maps. In our opinion, LczExplore found a place there. For instance in Bernard et al. (2023) manuscript, LczExplore was useful to compare OSM and BDTopo input data. We saw that the main discrepancies reflect the way building heights are taken into account but also the differences of input vegetation data in OSM and BDTopo. These are relevant informations that have been highlighted using the LczExplore package and it can be used the same way to compare any LCZ map (for example GeoClimate to WUDAPT) for a given territory.
Another useful feature of the package is how easy it makes grouping LCZ types into broader categories : if one wants to compare the urban envelopes of an area, it is very straightforward to group, for instance, all the urban LCZ, all the vegetation LCZ and so on, and to compare the resulting map, all in one go.
About the State of the field:
In our understanding, JOSS papers had to focus on similar tools, i.e. tools to compare LCZ classifications and not tools that produce LCZ classifications. This is why we did not describe any LCZ classification tool in details. We do not know any free and open source tool for comparing two LCZ maps while it seems interesting to offer such service. If you know more about such tool, please let us know.
The package is specifically designed for LCZ in the sense that when you specify that repr='standard' the colors on maps are standard LCZ map style colors. However, with repr='grouped' any qualitative variable pair of maps can be compared, as you can specify the names of the levels and the colors you want them to be associated with. A particular effort has been made to make this choice of grouping and colors robust for the user, with error messages quite explicit, so it may help geomaticians to compare several outputs for their treatments without any tedious repetitive work.
Representation: In our opinion, the neighborhood-scale averages informations that might be quite heterogeneous at block scale. The way to compare WUDAPT to GeoClimate is not set yet since GeoClimate also allows to aggregate the LCZ available at block scale to the WUDAPT grid cell, keeping at this scale an information of the degree of heterogeneity found at block scale. In any case (the block information being aggregated at cell scale or not), LczExplore can still be used to investigate the differences between maps resulting from the GeoClimate or the WUDAPT approach in a near future.
About coding standards: You are right about age of the tool, it has been developped for the need of Bernard et al. (2023) manuscript and has not been tested extensively since then. But it has been tested and shows interesting results when comparing WUDAPT to GeoClimate maps. The package has no warning and no error when one runs R CMD CHECK (only notes, which is allowed by CRAN standards). Each function has unitary tests (one can explore in the /inst/tinytest folder of the package) which are all documented and a vignette shows the most probable usecase.
Concerning the documentation and the readme informations: We agree that some concretes examples must be added to facilitate the use of LczExplore. We will extend and describe it a bit more and specify how to sequence the main functions to produce the comparison of two maps. Concretely we propose to describe the way to obtain a comparison of two vector layer maps (such as in Bernard et al. (2023) manuscript plus also a new one describing how to obtain the comparison of European WUDAPT tiff LCZ map to a GeoClimate OSM output.
Whatever the choice of JOSS regarding this paper, we are looking forward to deepening the comparison of several approaches of LCZ generation, including, of course, the WUDAPT and GeoClimate approaches.
matthiasdemuzere
commented
1 year ago @MGousseff Thanks for providing additional information regarding my concerns.
Unfortunately I am still not convinced by the statement of need.
There are nowadays hundreds of papers using LCZs for a certain purpose. Yet, typically, people are not interested in comparing one LCZ map to another (typically even not in a proper accuracy assessment of one map). They just use the LCZ map they (or someone else) created (using whatever algorithm / input data) in any application of interest.
There are some that do an intercomparison, like the Muhammad et al (2022) paper you mention in your manuscript. In which they conclude that the GIS-LCZ based method shows a strong improvement over the previous WUDAPT L0 result, based on accuracy metrics (confusion matrix). We've checked this map in detail with local experts, and believe it is missing important features, eg. large LCZ 8 zones. So even though accuracy metrics can be higher, that does not necessarily mean that map is better.
Continuing with this example, I could use your lczexplore tool to visualize differences between the Muhammad et al. (2022) LCZ map and e.g. the LCZ map from Fenner et al. (2017). But then what? You mention the identification of how different input datasets treat vegetation, building heights, ... differently. Ok. Good. But again, what then?
So basically I am still looking for a more clear statement of need that outlines the potential of this tool to a broad community. How can it contribute to an improved understanding of the uncertainties, strengths and weakness of the LCZ framework / method(s)? Identifying differences between maps is one thing, but the crucial step is what comes after that: what to do with this information?
I hope I am not sounding to cruel here. I just genuinly would like to understand how this will benefit the community, in a broader sense and a context of continuesly advancing the field of LCZ-related work.
matthiasdemuzere
commented
1 year ago plus also a new one describing how to obtain the comparison of European WUDAPT tiff LCZ map to a GeoClimate OSM output.
This would be much appreciated. I started using the tool, but I stopped as it was not 100% clear to me how to do so with a raster file. There was some very high level info there, but in my opinion that required too much time investment. Given the selling position this is a tool, it should in my opinion be much more automated and self-explanatory?
On the side: how does the tool deal with different labels? LCZ labels can very widely, example for the natural classes 11-17 or A - G or 101-107, ... More general: a large proportion of the LCZ Generator and W2W code is dedicated to checking inputs, to make sure they are in line with expected formats. Does lczexplore has something similar in place?
wcjochem
commented
1 year ago @MGousseff and @martinfleis, I've raised several issues in the repo (and linked in this thread) for your consideration and have completed my review at this stage. I can see that the package does support its primary aim, although it seems to be a very narrow application. I can't verify the claim in the paper that the package's functionality would be useful more generally in other types of spatial comparisons. I'm happy to continue my review if these comments are addressed.
MGousseff
commented
1 year ago @wcjochem Thank you very much for all your issues.
I really thank you and @matthiasdemuzere and I will deliver as soon as possible a new version with all these issues fixed.
MGousseff
commented
1 year ago @wcjochem , @matthiasdemuzere: Thank you for all the improvements you suggested. It took a little time but we think we addressed all of those improvements in this new version of the paper. Here are the points you raised and what we did about them.
About the statement of need: we added references to some articles of map comparisons for environmental purpose. It led us to add a pseudo Cohen's Kappa coefficient that takes into account the part of agreement that could occur by chance.
The question of what to do once differences have been highlighted is beyond the scope of this package/article, but lczexplore helps identify which steps of an algorithm need further investigation (in our example, one is the estimation of building height by an algorithm, and another, is the quality of the vegetation data).
About the LCZ labels variability : the ones we met are taken into account by the standard representation :
1 to 10 then 11 to 17
1 to 10 then 101 to 107
1 to 10 then A to G
For other standards, see next point.
A specific function was needed to import easily any qualitative variable: we added the importQualVar function. It also allows to import LCZ classifications whose levels would not be expected in importLCZvect and importLCZraster functions.
About coding standard : The only pipe left is the tidyr one, as it is easier for retro-compatibility The devtools::check_installed function only outputs 0 ERROR, 0 WARNING and 2 NOTES. The notes are related to the example data: we chose to keep in the package full areas in order to have illustrative examples. No note is related to code anymore. We used the Renv package to help users know which versions of packages we used. Package folder has a Renv.lock. Using the renv::status() gives the version of packages when the sources were built.
The vignettes are now properly built. "lczexplore_raster_vector" vignette is dedicated to the comparison of a WUDAPT like LCZ map and a GeoClimate Like Vector map.
We recommand to start with vignette("lczexplore_en")
We thank you as your comments really helped to improve lczexplore package and we hope we answered your main concerns about this paper.
wcjochem
commented
1 year ago @editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
matthiasdemuzere
commented
1 year ago @MGousseff, thanks for revising this submission.
For starters, I tried to install the package with vignetting:
library(devtools)
devtools::install_github("orbisgis/lczexplore", build_vignettes = TRUE)
library(lczexplore)Unfortunately this fails on my system, with the following complaints:
✔ checking for file ‘/tmp/RtmpE8qrsq/remotes13db89663a5a/orbisgis-lczexplore-fa35bd9/DESCRIPTION’ ...
─ preparing ‘lczexplore’:
✔ checking DESCRIPTION meta-information ...
─ installing the package to build vignettes
E creating vignettes (19.2s)
--- re-building ‘lczexplore_alter.Rmd’ using knitr
Quitting from lines 15-24 [unnamed-chunk-1] (lczexplore_alter.Rmd)
Error: processing vignette 'lczexplore_alter.Rmd' failed with diagnostics:
trying to use CRAN without setting a mirror
--- failed re-building ‘lczexplore_alter.Rmd’
--- re-building ‘lczexplore_en.Rmd’ using knitr
Quitting from lines 15-24 [unnamed-chunk-1] (lczexplore_en.Rmd)
Error: processing vignette 'lczexplore_en.Rmd' failed with diagnostics:
trying to use CRAN without setting a mirror
--- failed re-building ‘lczexplore_en.Rmd’
--- re-building ‘lczexplore_fr.Rmd’ using knitr
Quitting from lines 17-27 [unnamed-chunk-1] (lczexplore_fr.Rmd)
Error: processing vignette 'lczexplore_fr.Rmd' failed with diagnostics:
trying to use CRAN without setting a mirror
--- failed re-building ‘lczexplore_fr.Rmd’
--- re-building ‘lczexplore_raster_vector.Rmd’ using knitr
Quitting from lines 15-24 [unnamed-chunk-1] (lczexplore_raster_vector.Rmd)
Error: processing vignette 'lczexplore_raster_vector.Rmd' failed with diagnostics:
trying to use CRAN without setting a mirror
--- failed re-building ‘lczexplore_raster_vector.Rmd’
SUMMARY: processing the following files failed:
‘lczexplore_alter.Rmd’ ‘lczexplore_en.Rmd’ ‘lczexplore_fr.Rmd’
‘lczexplore_raster_vector.Rmd’
Error: Vignette re-building failed.
Execution halted
Error: Failed to install 'lczexplore' from GitHub:
! System command 'R' failedI am not sure why this is. To be honest, I have not used R for a very long time. So I did a clean install on my ubuntu 20.04 64bit system, with this version in place:
> version
_
platform x86_64-pc-linux-gnu
arch x86_64
os linux-gnu
system x86_64, linux-gnu
status
major 4
minor 3.1
year 2023
month 06
day 16
svn rev 84548
language R
version.string R version 4.3.1 (2023-06-16)
nickname Beagle ScoutsFYI: I am about to take a summer break, and I will only be able to look into this again from September onwards. My apologies for the delay this may cause.
MGousseff
commented
1 year ago @matthiasdemuzere Thank you for trying to install this version. I tried to install the package with these instruction on my home computer without this error.
The vignettes use a package called png, and the instruction at line 15-24 of the vignette is to check if it is installed and if not, to try and download it. Maybe there is a problem in the way CRAN package are downloaded ? I'll try from another computer which doesn't have any R version on it, and I'll install everything from scratch to see if I can reproduce the problem.
Maybe you can try to install knitr and png from your session before you install/load lczexplore ?
No problem for the delay, I am actually on holidays too, I'll just take the time to check from a test computer I keep to this end at home.
I hope we manage to fix this soon to see if the changes make the package more useful for you and your team. Enjoy your summer.
MGousseff
commented
1 year ago Dear @matthiasdemuzere, the error comes from the fact you haven't specified a CRAN mirror in your session. RStudio does it automatically in interactive mode, but not when a vignette is built, and I integrated the installation of the png package to allow better visualizations in the vignette. I have fixed this by specifying a CRAN mirror in the vignette code. If you try the install with this code
library(devtools)
devtools::install_github("orbisgis/lczexplore")
library(lczexplore)
it should install without problems. If you use RStudio, vignette("lczexplore_en") should offer a quick overview. But the code in the article must work as easily.
I also modified the graphics of the confidence analysis, for readability reasons.
I hope I understood your concerns and improved the package/article (for instance with other qualitative variable import and exploration) and you find some use of the actual version of the package.
MGousseff
commented
11 months ago @wcjochem : Do you encounter any similar problems installing or testing the package ? Please let me know if I can help (or maybe I missed a github/JOSS notification ? I'm still new to this reviewing process).
wcjochem
commented
11 months ago @MGousseff I have been able to install the latest version of the package and build/run the vignettes with no errors. I have reviewed the latest draft of the paper. I see that you have closed most (but not all) the issues I raised in the repository. For future reference, I think it's preferable if you respond to the issues stating what you did to address it (or why you won't address it) before closing it. I have updated my checklist to reflect your updates.
MGousseff
commented
11 months ago I'm sorry for closing the issue too fast. The different threads (reviews and Issues) was a bit confusing to me, I seeked advice and followed a colleague point of view of closing the issue after flagging them done. If I'm not mistaken, the last Issue still open is about 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support? What would be the best way to improve this in your opinion ? Would it be to add my contact info in the readme ? As per your review, I thought it would appear here for me to take into account the improvements you may have asked ? (Thanks for your patience, still not used to which thread to follow).
wcjochem
commented
11 months ago What would be the best way to improve this in your opinion ? Would it be to add my contact info in the readme ?
This item is specifically listed in the JOSS checklist (https://joss.readthedocs.io/en/latest/review_checklist.html). It's up to you to decide how you want users to interact with your project. I suggest you check other JOSS published packages to see how they handle it. Some people add a section to their README, others add .github/CONTRIBUTING.md, others have other options.
MGousseff
commented
11 months ago Thank you for helping me improve my practice. I added contributing and code of conduct files. I let the issue opened so that you can confirm it's okay. Thank you for helping me improve my practices.
martinfleis
commented
11 months ago Hi @matthiasdemuzere, do you have an estimation on when you can get back to the review? Is there anything blocking you from @MGousseff's side?
matthiasdemuzere
commented
11 months ago Hi @martinfleis, my apologies this is taking me so long. I am currently transitioning from academia to being self-employed, and this is consuming more of my time than anticipated.
Excuses aside, I'll try to deliver before mid October!
MGousseff
commented
10 months ago @wcjochem Can I close the issue about contributing ? @matthiasdemuzere, I have been self employed a decade ago, I wish you good luck for the redtape. Let me know if some modifications are needed.
wcjochem
commented
10 months ago @wcjochem Can I close the issue about contributing ?
@MGousseff, I believe I closed that issue earlier after you made the change. Because I included a link between that issue and this thread you can see in this thread (above) that the issue is closed in your repository. Thanks for the update and your work to resolve these issues.
MGousseff
commented
10 months ago I should have checked the issue was closed (should have thought you could close it by yourself as the creator of the issue). Is there anything you need now in order to review the package ? Thanks for all the improvements.
martinfleis
commented
10 months ago @matthiasdemuzere a gentle ping that mid October has creeped upon us ;)
matthiasdemuzere
commented
10 months ago Dear @matthiasdemuzere, the error comes from the fact you haven't specified a CRAN mirror in your session. RStudio does it automatically in interactive mode, but not when a vignette is built, and I integrated the installation of the png package to allow better visualizations in the vignette. I have fixed this by specifying a CRAN mirror in the vignette code. If you try the install with this code
library(devtools)devtools::install_github("orbisgis/lczexplore")library(lczexplore)it should install without problems. If you use RStudio,
vignette("lczexplore_en")should offer a quick overview. But the code in the article must work as easily.I also modified the graphics of the confidence analysis, for readability reasons.
I hope I understood your concerns and improved the package/article (for instance with other qualitative variable import and exploration) and you find some use of the actual version of the package.
Hi @MGousseff,
I am on a new laptop, and had to install the R environment again.
The errors below are generated using R - 4.1.2
This time I also manually installed knitrand png. And I have set a CRAN mirror (when I was using Rstudio)
But I still get the same error when building the vignettes. Not only in Rstudio, yet also in a terminal.
> devtools::install_github("orbisgis/lczexplore", build_vignettes = TRUE, force=TRUE)
Downloading GitHub repo orbisgis/lczexplore@HEAD
── R CMD build ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
✔ checking for file ‘/tmp/RtmppoV2C5/remotes1083c3844a275/orbisgis-lczexplore-ca6c8ee/DESCRIPTION’ ...
─ preparing ‘lczexplore’:
✔ checking DESCRIPTION meta-information ...
─ installing the package to build vignettes
E creating vignettes (1m 8.4s)
--- re-building ‘lczexplore_alter.Rmd’ using knitr
Error: processing vignette 'lczexplore_alter.Rmd' failed with diagnostics:
'mark_html' is not an exported object from 'namespace:markdown'
--- failed re-building ‘lczexplore_alter.Rmd’
--- re-building ‘lczexplore_en.Rmd’ using knitr
Error: processing vignette 'lczexplore_en.Rmd' failed with diagnostics:
'mark_html' is not an exported object from 'namespace:markdown'
--- failed re-building ‘lczexplore_en.Rmd’
--- re-building ‘lczexplore_fr.Rmd’ using knitr
Error: processing vignette 'lczexplore_fr.Rmd' failed with diagnostics:
'mark_html' is not an exported object from 'namespace:markdown'
--- failed re-building ‘lczexplore_fr.Rmd’
--- re-building ‘lczexplore_raster_vector.Rmd’ using knitr
Error: processing vignette 'lczexplore_raster_vector.Rmd' failed with diagnostics:
'mark_html' is not an exported object from 'namespace:markdown'
--- failed re-building ‘lczexplore_raster_vector.Rmd’
SUMMARY: processing the following files failed:
‘lczexplore_alter.Rmd’ ‘lczexplore_en.Rmd’ ‘lczexplore_fr.Rmd’
‘lczexplore_raster_vector.Rmd’
Error: Vignette re-building failed.
Execution halted
Error: Failed to install 'lczexplore' from GitHub:
! System command 'R' failedMore info on the R-version on my ubuntu 22.04:
> version
_
platform x86_64-pc-linux-gnu
arch x86_64
os linux-gnu
system x86_64, linux-gnu
status
major 4
minor 1.2
year 2021
month 11
day 01
svn rev 81115
language R
version.string R version 4.1.2 (2021-11-01)
nickname Bird Hippie For now I true to continue the review without building the vignettes, since this seems to work ...
> devtools::install_github("orbisgis/lczexplore", build_vignettes = FALSE, force=TRUE)
Downloading GitHub repo orbisgis/lczexplore@HEAD
── R CMD build ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
✔ checking for file ‘/tmp/RtmpYLe5LO/remotesddf41d046fb6/orbisgis-lczexplore-ca6c8ee/DESCRIPTION’ ...
─ preparing ‘lczexplore’:
✔ checking DESCRIPTION meta-information ...
─ checking for LF line-endings in source and make files and shell scripts
─ checking for empty or unneeded directories
─ building ‘lczexplore_0.0.1.0002.tar.gz’
Installing package into ‘/home/matthias/R/x86_64-pc-linux-gnu-library/4.1’
(as ‘lib’ is unspecified)
* installing *source* package ‘lczexplore’ ...
** using staged installation
** R
** data
*** moving datasets to lazyload DB
** inst
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** installing vignettes
** testing if installed package can be loaded from temporary location
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
* DONE (lczexplore)
> library(lczexplore)
Hi @MGousseff,
From here onwards, I'll go over the open check-boxes from my check list, and provide some information to what I think can still be improved. For simplicity and clarity, I will use a comment per check-box.
matthiasdemuzere
commented
10 months ago -Installation: Does installation proceed as outlined in the documentation?
The installation works, except when I try to build the vignette, as explained here. This was the case when I was using R version 4.1.2.
Now, I noticed you have a renv.lock file in your repo, that - I assume (remember I am no R user) - sets the right environment and dependencies. In that file you refer to R version 4.3.0, so I installed this on my system.
After setting this as the default, I proceeded as listed in the JOSS paper:
library(devtools)
devtools::install_github("orbisgis/lczexplore", build_vignettes = TRUE)But this still fails, even though the error messages are now different. It now seems to relate to X11 fonts? Not sure what this means, but perhaps you can look into this, to ensure a smooth installation for users on different OSs?
devtools::install_github("orbisgis/lczexplore", build_vignettes = TRUE)
Loading required package: usethis
Downloading GitHub repo orbisgis/lczexplore@HEAD
── R CMD build ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
✔ checking for file ‘/tmp/RtmpNU1R5i/remotes2461c154335df/orbisgis-lczexplore-ca6c8ee/DESCRIPTION’ ...
─ preparing ‘lczexplore’:
✔ checking DESCRIPTION meta-information ...
─ installing the package to build vignettes
E creating vignettes (20.9s)
--- re-building ‘lczexplore_alter.Rmd’ using knitr
trying URL 'http://cran.us.r-project.org/src/contrib/png_0.1-8.tar.gz'
Content type 'application/x-gzip' length 24880 bytes (24 KB)
==================================================
downloaded 24 KB
─ installing *source* package ‘png’ ...
** package ‘png’ successfully unpacked and MD5 sums checked
** using staged installation
** libs
using C compiler: ‘gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0’
gcc -I"/home/matthias/R/x86_64-pc-linux-gnu-library/4.3.0/lib/R/include" -DNDEBUG -I/usr/local/include `libpng-config --cflags` -fpic -g -O2 -Wall -pedantic -fdiagnostics-color=always -c dummy.c -o dummy.o
gcc -I"/home/matthias/R/x86_64-pc-linux-gnu-library/4.3.0/lib/R/include" -DNDEBUG -I/usr/local/include `libpng-config --cflags` -fpic -g -O2 -Wall -pedantic -fdiagnostics-color=always -c read.c -o read.o
gcc -I"/home/matthias/R/x86_64-pc-linux-gnu-library/4.3.0/lib/R/include" -DNDEBUG -I/usr/local/include `libpng-config --cflags` -fpic -g -O2 -Wall -pedantic -fdiagnostics-color=always -c write.c -o write.o
gcc -shared -L/home/matthias/R/x86_64-pc-linux-gnu-library/4.3.0/lib/R/lib -L/usr/local/lib -o png.so dummy.o read.o write.o -lpng16 -lm -lz -lm -L/home/matthias/R/x86_64-pc-linux-gnu-library/4.3.0/lib/R/lib -lR
installing to /tmp/RtmpnG3Y0q/Rinst31c918bace90/00LOCK-png/00new/png/libs
** R
** inst
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** testing if installed package can be loaded from temporary location
** checking absolute paths in shared objects and dynamic libraries
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
─ DONE (png)
The downloaded source packages are in
'/tmp/RtmpvUcrVy/downloaded_packages'
Quitting from lines 62-67 [unnamed-chunk-3] (lczexplore_alter.Rmd)
Error: processing vignette 'lczexplore_alter.Rmd' failed with diagnostics:
X11 font -adobe-helvetica-%s-%s-*-*-%d-*-*-*-*-*-*-*, face 1 at size 9 could not be loaded
--- failed re-building ‘lczexplore_alter.Rmd’
--- re-building ‘lczexplore_en.Rmd’ using knitr
Quitting from lines 104-105 [unnamed-chunk-4] (lczexplore_en.Rmd)
Error: processing vignette 'lczexplore_en.Rmd' failed with diagnostics:
X11 font -adobe-helvetica-%s-%s-*-*-%d-*-*-*-*-*-*-*, face 1 at size 9 could not be loaded
--- failed re-building ‘lczexplore_en.Rmd’
--- re-building ‘lczexplore_fr.Rmd’ using knitr
Quitting from lines 95-96 [unnamed-chunk-4] (lczexplore_fr.Rmd)
Error: processing vignette 'lczexplore_fr.Rmd' failed with diagnostics:
X11 font -adobe-helvetica-%s-%s-*-*-%d-*-*-*-*-*-*-*, face 1 at size 9 could not be loaded
--- failed re-building ‘lczexplore_fr.Rmd’
--- re-building ‘lczexplore_raster_vector.Rmd’ using knitr
Quitting from lines 93-94 [unnamed-chunk-7] (lczexplore_raster_vector.Rmd)
Error: processing vignette 'lczexplore_raster_vector.Rmd' failed with diagnostics:
X11 font -adobe-helvetica-%s-%s-*-*-%d-*-*-*-*-*-*-*, face 1 at size 9 could not be loaded
--- failed re-building ‘lczexplore_raster_vector.Rmd’
SUMMARY: processing the following files failed:
‘lczexplore_alter.Rmd’ ‘lczexplore_en.Rmd’ ‘lczexplore_fr.Rmd’
‘lczexplore_raster_vector.Rmd’
Error: Vignette re-building failed.
Execution halted
Error: Failed to install 'lczexplore' from GitHub:
! System command 'R' failed-Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
There is an extensive list of dependencies. But related to the comment above, perhaps png and knitr should be listed there as well? Preferably, the package installs all the dependencies by default, to easen the life of a user? See also comment right above this one.
matthiasdemuzere
commented
10 months ago - Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support
I don't see any community guidelines?
MGousseff
commented
10 months ago Dear @matthiasdemuzere, Thank you for finding the time to review again. I am so sorry this problem of vignettes makes you lose time. It is not the same error message as last time, and I can't reproduce it on my computer. I am currently on the same ubuntu version so I can't see what is the hiatus. I am setting a virtualbox image to check from start and see if I can reproduce the bug. I'll tell you as soon as I find a fix if I can reproduce. Sorry for this and thank you for the time spent.
matthiasdemuzere
commented
10 months ago - Software paper: Summary & Statement of need
For simplicity I prefer to address these sections together.
Now, I realize we already had an extensive discussion on the need for a tool to compare LCZ maps. I don't want to go back there, and will leave it up to the user community to decide whether this is useful.
But.
I do think the framing can be improved.
Given that the name of the package is lczexplore and all examples are provided on LCZ data, it probably makes more sense to focus both the Summary & Statement of need sections on LCZ comparisons.
Of course you could still add applications for any intercomparison, but then you should also add a random example (including data) on this.
MGousseff
commented
10 months ago - Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support
I don't see any community guidelines?
They are in the .github folder. I was told it is standard practice to put them there, but I can move them to the package root if you find it more convenient ?
matthiasdemuzere
commented
10 months ago - Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support I don't see any community guidelines?
They are in the .github folder. I was told it is standard practice to put them there, but I can move them to the package root if you find it more convenient ?
Ha, I missed that. I am also unware about this "standard practice".
Personally I find the info not very visible as such? Perhaps better to have it in the README.md itself? But I leave this up to you. I'll check this box now.
martinfleis
commented
10 months ago I don't see any community guidelines?
They are in the .github folder. I was told it is standard practice to put them there, but I can move them to the package root if you find it more convenient ?
Ha, I missed that. I am also unware about this "standard practice".
Neither contributing nor code of conduct shall be in the .github folder. That is intended for interaction with GitHub APIs, not users or contributors. The optimal place is the root of the repository.
Submitting author: !--author-handle-->@MGousseff<!--end-author-handle-- (Matthieu Gousseff) Repository: https://github.com/orbisgis/lczexplore Branch with paper.md (empty if default branch): master Version: 0.0.1.0003 Editor: !--editor-->@martinfleis<!--end-editor-- Reviewers: @matthiasdemuzere, @wcjochem Archive: 10.5281/zenodo.10041206
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@matthiasdemuzere & @wcjochem, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @martinfleis know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Checklists
📝 Checklist for @wcjochem
📝 Checklist for @matthiasdemuzere