DOI-USGS / hyRefactor

https://code.usgs.gov/wma/nhgf/reference-fabric/hyrefactor
Creative Commons Zero v1.0 Universal
5 stars 0 forks source link

Conflicting function names for aggregation #40

Open mikejohnson51 opened 2 years ago

mikejohnson51 commented 2 years ago

Both hyRefactor and hyAggregate have an aggregate_network function. I think there are two solutions to provide more clarity.

Since hyRefactor is a hyAggregate dependency, the point based aggregation workflow could be moved to hyAggregate and the aggregation driven by the input parameters. This would result in a signature like:

aggregate_network = function(gf = NULL,
                             flowpaths  = NULL,
                             catchments = NULL,
                             ideal_size_sqkm = 10,
                             min_length_km = 1,
                             min_area_sqkm  = 3,
                             outfile = NULL,
                             verbose = TRUE,
                             overwrite = FALSE,
                             nexus_topology = TRUE,
                             routelink_path = NULL,
                             outlets,
                             only_larger = FALSE) {

... 
}

Or, the functions could be renamed to something like: aggregate_network_to_distribution and aggregate_network_to_outlets and either remain in the separate packages, or, consolidate into hyAggregate.

Do you have any thoughts?

Mike

dblodgett-usgs commented 2 years ago

I'd be happy to have it move to hyAggregate -- honestly, I think the packages could combine long run.

mikejohnson51 commented 2 years ago

I agree - next time we talk this would be any interesting point to discuss. The hyAggregate dependency list has bloated a bit to support nextgen specific tasks. It might be time to bring hyRelease back into the mix...

Either way, you want me to bring the hyRefactor function over or do you want to submit a PR? I have a big push coming this week so there is no urgency.