-
Notifications
You must be signed in to change notification settings - Fork 2
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
Improvements arising from suggestions evt #132
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 from me, thoughts @e-kotov ?
@Robinlovelace Wondering why ubuntu devel suddenly fails, though @eugenividal did not touch the code ) Probably has something to do with ubuntu latest moving to v24 in github actions... Have not studied the logs yet. @eugenividal are you hesitating to touch the code and the docs? Some of the changes in the vignette are consistent with the package docs and argument options. 'Population by trip count data' - I'm not sure this reads well. In the package docs we refer to it as "number_of_trips" (see help for spod_get, spod_download, spod_convert). So if we change it in vignette, we need to make it consistent with the package options and docs. But the package is already released. Not that many people have downloaded it, but I would be hesitant to suddenly change the arguments and make breaking changes. So at this point I am hesitant to change the vignette in this way, especially if the new heading does not read well (to me at least it does not). One of the problems is this specific data set that has this weird form/grouping of number or people by number of trips as a factor... Could we try alternative headings for this one that would be easy to understand but also consistent with the docs? |
Hi @e-kotov, yeah, not sure what happened with this PR. |
@eugenividal I checked the main branch and the tests are failing for the main as well, the reason is some bug that I will attend later, it's not critical. This 'number of trips' is super confusing and hard to explain, but I think we should try ) I would ask for some patience and allow me some more time (about a week from now, as I'm a bit busy right now) to add to/review your changes as they may require changes in the package docs and I want it to be a cohesive thing before merging, I don't want to push to the main something that will be immediately reflected on the package website and may not represent what the CRAN version of the package does and says in the docs. |
@e-kotov, absolutely, no rush and agree with your comments, better not to change anything unless it is a clear and worth it improvement! The package is already great. These are just minor suggestions that we can implement only if they make it better and do not represent big changes. I agree, the 'number of trips' is quite confusing! :) But we'll find a way to clarify it. The dataset is really complex, and sometimes complexity can't be easily simplified. I just did this experiment: I asked ChatGPT to suggest titles for the maestra 1 and 2 tables of version 1, but I think it struggled too. Here are the responses:
I wouldn't say maestra 2 provides the 'Number of Trips and People by District and Trip Category', but only the 'Number People by District and Trip Category'. |
@e-kotov and @Robinlovelace, in this PR I've:
In the README:
Rephrased the paragraph explaining the different versions of the data based on the improvements on the paper.
In the vignettes:
Renamed the mobility datasets as follows:
+ Version 1, Mobility data:
- Origin-Destination data
- Population by trip count data
+ Version 2, Mobility data:
- Origin-Destination data
- Population by trip count data
- Population by overnight stay data
These were the main tasks from #129. As for the rest, I found some issues:
In the README:
Given that the title is done with one #, add one level (i.e. ##) to the headings 1. I've tried this but there are several
qmd
files linked to the README, so to make it work we would have to lower one level in all the files, or we can just forget about this very minor issue. It is just aestheticsplot the desire lines using the flow mapper.
Not sure where this plot comes from
Remove the title of the first figure --- there is already a caption with title for this figure.
Same as above
Beautify Figure 4, checking for typos (e.g. munucipality, houlry), putting the first three boxes at the same level, etc.
Same as above
In the package:
These suggestions regarding function names were also very minor. If you agree, better if you implement them @e-kotov. I am not sure how to rename functions.
Fix inconsistencies in the name of the variables of the different versions: e.g. date and full_date, and residence_province_name and residence_province.
I'd consider renaming some variables: E.g. time slot = hour_slot and distance = interval_distance