Opened 12 months ago

Last modified 3 months ago

#22423 assigned enhancement

Refactor R code to use modern R packages and methods

Reported by: johnbwilliams Owned by: metrics-team
Priority: High Milestone:
Component: Metrics/Website Version:
Severity: Normal Keywords:
Cc: metrics-team Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

plot_networksize() has been refactored to use modern R packages & methods,
resulting in readable, maintainable code. Details here: http://rpubs.com/johnbwilliams/refactor. Minor adjustments are needed to drop in place. Package dependencecy considerations apply.

Child Tickets

Change History (11)

comment:1 Changed 8 months ago by karsten

Summary: plot_networksize() refactorRefactor plot_networksize() to use modern R packages and methods

Tweak summary.

comment:2 Changed 4 months ago by karsten

Owner: changed from metrics-team to karsten
Status: newaccepted
Summary: Refactor plot_networksize() to use modern R packages and methodsRefactor R code to use modern R packages and methods

I'll pick this up and extend it to all R code we have, after writing more recent R code with help of dplyr and tidyr.

johnbwilliams, if you're still around, maybe you'd like to review the changes?

comment:3 Changed 4 months ago by karsten

I started making changes and pushing them to master. No need to do the full review process before merging here, it's just presentation. I'll make more changes over the next few days.

comment:4 Changed 3 months ago by karsten

Owner: changed from karsten to metrics-team
Status: acceptedassigned

I made more changes, but I need to pause for a while and do other stuff now. In the meantime I'm giving this ticket back to the metrics-team pool.

If somebody feels like reviewing some R code, please take a look at the three functions at the end of graphs.R, namely plot_relays_ipv6, plot_bridges_ipv6, and plot_advbw_ipv6: https://gitweb.torproject.org/metrics-web.git/tree/src/main/R/rserver/graphs.R#n1097 ; these would then serve as blueprints for the other graphs.

comment:5 Changed 3 months ago by iwakeh

Owner: changed from metrics-team to iwakeh

Grabbing the review to also push #24707 a little.

comment:6 Changed 3 months ago by karsten

Priority: MediumHigh
Severity: MinorNormal

Raising priority to high, because I'd like to continue refactoring some more R code in parallel to documenting it for our Sponsor 13 deliverable.

comment:7 Changed 3 months ago by karsten

Cc: metrics-team added
Keywords: plot_networksize() removed

comment:8 Changed 3 months ago by karsten

FWIW, I just pushed two more commits to master.

comment:9 Changed 3 months ago by iwakeh

Assuming the committed code already got tested regarding functionality and calculates the correct results I checked for readability and the like.
The modernization makes all much more readable. What hinders here and there is the line breaks and indentation, but that will be work after the style guide is settled (cf. #24707).

Also the additional two commits look fine.

comment:10 Changed 3 months ago by iwakeh

Status: assignedmerge_ready

Well, it is merged already.

comment:11 Changed 3 months ago by karsten

Owner: changed from iwakeh to metrics-team
Status: merge_readyassigned

Thanks for looking!

Some functions still use the old code style, so I'd say let's leave this ticket open for now. I'll un-assign it, though, because I'm done for the moment. Feel free to grab it back.

And yes, let's tackle the indentation stuff in #24707.

Note: See TracTickets for help on using tickets.