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

Chapter 2 code review #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Chapter 2 code review #3

wants to merge 2 commits into from

Conversation

cforgaci
Copy link
Collaborator

@cforgaci cforgaci commented Apr 10, 2024

I made some suggestions on the code for Chapter 2.

@bayi, I could not run Section 3 as I did not have the latest data for it. If you share it, I can also review that section as part of this PR.

@@ -1,20 +1,26 @@
### Code to exemplify Part 2 with Xiaoxia's query

# Install and load packages ----
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved all package loading and installation to the beginning of the script. Packages are only installed if they are not present on the user's system.

Comment on lines +153 to +155
# Remove entries without a title
bib_data <- bib_data[bib_data$Title != "", ]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hre I removed the entries without a title. That seems to have caused issues later in the BibEntry list.

@@ -144,6 +150,9 @@ bib_data$Volume <- gsub("'", "", bib_data$Volume)
bib_data$Number <- gsub("'", "", bib_data$Number)
bib_data$Pages <- gsub("'", "", bib_data$Pages)

# Remove entries without a title
bib_data <- bib_data[bib_data$Title != "", ]

# Create a list of BibEntry objects
# (l got problems that l cannot extract title from those cases:title="'Inner-city is not the place for social housing' - State-led gentrification in Lodz" and '"Freeways without futures": Urban highway removal in the United States and Spain as socio-ecological fix?' )
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does my addition above solve this issue or is this something else?

Comment on lines +235 to +237
# Remove entries without a title
combined_data <- combined_data[combined_data$Title != "", ]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, entries without title removed.

#install.packages("PRISMAstatement")
library(PRISMAstatement)

# Section 4: build PRISMA figure (Xiaoxia) ----
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this section is unnecessarily long. It would be enough to show a full example with all/most customisation and refer the reader to the package documentation.

# install_github("elizagrames/litsearchr", ref="main")
library(litsearchr)
library(igraph)
# Section 3: Suggest new keywords (Clementine) ----
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I so not have the data for this section, so I cannot run it.

@@ -408,7 +416,7 @@ prisma(found = 750,
width = 200, height = 200,
dpi = 36)

## ----prismapdf, echo = TRUE, eval = FALSE--------------------------------
## Prismapdf
Copy link
Collaborator Author

@cforgaci cforgaci Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this section be removed?

@cforgaci cforgaci requested a review from valaneila April 11, 2024 06:02
@cforgaci
Copy link
Collaborator Author

@hadyyu, @valaneila, while talking to @ClementineCttn today I noticed that my review is on the wrong file, the R script, and it should be on the Rmd. I discussed the main points of my feeback with @ClementineCttn, but I will also check the Rmd file to see if I have any other suggestion. When I'm done I will leave an update here.

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

Successfully merging this pull request may close these issues.

1 participant