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

Conflicting function names for aggregation #40

Open
mikejohnson51 opened this issue Jun 8, 2022 · 2 comments
Open

Conflicting function names for aggregation #40

mikejohnson51 opened this issue Jun 8, 2022 · 2 comments

Comments

@mikejohnson51
Copy link
Contributor

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

@mikejohnson51 mikejohnson51 changed the title Conflicting names Conflicting function names for aggregation Jun 8, 2022
@dblodgett-usgs
Copy link
Collaborator

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

@mikejohnson51
Copy link
Contributor Author

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.

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

No branches or pull requests

2 participants