Implement the RoutesDataClass as a generic data format #204

chenyangkang · 2025-01-18T18:56:41Z

Minor changes:

Remove bridflow_crs and related. Delete old birdflow_crs object #203
Add functions (initial) for calculating distance metrics Implement a new model validation method: The predictive distance metric #201

…necessary. If bf not provided, use birdflow_crs in the environment. This will make data loading easier without loading the model.

…orted

…flow-science#202

…timestep 1 can cause inconsistency. So fix the random state.

… acquiring from BirdFlow models

chenyangkang · 2025-01-19T18:28:33Z

Seems to be a floating-point precision errors induced by %% operation. I changed it in another way using %/% and the problem seems to be solved. Very strange, might depend on system version.

…but not for date; the route generated by `route` function should be sorted

…e duplicated but date should not.

chenyangkang · 2025-01-20T14:39:58Z

@ethanplunkett Hi Ethan do you want to review the current changes before I go further to edit other functions? Especially about Routes, BirdFlowRoutes, BirdFlowIntervals (in RoutesDataClass.R), as_BirdFlowRoutes, as_BirdFlowIntervals (in RoutesDataFunction.R).

I still implemented the transformation function as_*, and kept the class functions open to dataframes that are already in the right format (like the dataframe generated by format_trajectory in the route function).

ethanplunkett · 2025-01-20T21:27:53Z

@chenyangkangyes. I want to review before it's merged. I have come down with something nasty though so am unlikely to get to it for a few days. I'm currently running a 102 fever.

Overall it's great to see you getting so much done!

I do have a couple thoughts based on what the code looked like Saturday morning when O last looked it over. Disregard anything that you've already addressed.

Ideally we'd have no new Notes in the package check. There were a couple that predate this update that you are unlikely to address.
When you align to birdflow while converting to 'BirdFlowRoutes' use 'snap_to-birdflow()' as that checks validity against the dynamic mask using the date and time simultaneously. It looked like your code was checking them independently which would allow locations that are excluded by the dynamic mask as long as they are in the static mask. I would expose the 'aggregate' argument too and add a 'random' option to mimic the default behavior of the new function.
We need to make sure 'plot_routes()' works with the modified routes. It's a bit of a mess so and the mess is all mine so let me know if you want me to clean it up.

chenyangkang · 2025-01-20T22:12:51Z

@ethanplunkett Sorry to hear that! And thanks for the quick feedback. I will check the first two points.

Regarding the plot_routes function, it seems to be working fine with the new class -- The data structure is not changed. Not sure if some hidden issues exist about that "crossing boundary" thing. I attached it and let me know if it doesn't look right. Also I'm thinking how about transforming (renaming) the plot_routes to a generic plot function on the BirdFlowRoutes and Routes class.

Hope you get better soon and we can discuss later!

ethanplunkett · 2025-01-20T23:58:50Z

Yes. It’s a good idea to make plot routes a method of the generic ‘plot’ now that we’ve formalized the class structure.

ethanplunkett

@chenyangkang I left lots of specific comments suggesting changes but overall this is great!

Some overarching comments:

Stylistically I prefer more succinct names. I don't think that matters enough to change it for the private functions and internal code, but it is worth shortening the public facing function and argument names.
I don't remember deciding whether we wanted to base these classes on data frames or lists. I do remember saying data frames might be cool. I like that they can then be treated like data frames but as you discovered manipulating them can lose the attributes, a problem people are less likely to bump up against with a list based object. Let's talk about this in person; I don't have a strong feeling either way but want to make sure we are making an informed decision.

R/RouteDataFunction.R

DESCRIPTION

ethanplunkett · 2025-01-21T20:22:45Z

NAMESPACE

 export(sparsify)
 export(species)
 export(species_info)
 export(truncate_birdflow)
 export(validate_BirdFlow)
+export(validate_BirdFlowIntervals_birdflow_intervals)


I think this should just be validate_BirdFlowIntervals and the following two exports should be validate_Routes and validate_BirdFlowRoutes.

Oh... I see there's also a validate_BirdFLowRoutes_geom etc. I still think the public validation functions shouldn't have the almost repeated text (_birdflow_route_df, route_df).

ethanplunkett · 2025-01-21T20:29:03Z

R/RouteDataClass.R

+
+#' @rdname RouteDataClass
+#' @export
+Routes <- function(route_df, species = NULL, metadata = NULL, source = NULL) {


Maybe change route_df to data or route_data or just df .

ethanplunkett · 2025-01-21T20:38:26Z

R/RouteDataFunction.R

+  routes |>
+    dplyr::group_by(route_type) |>
+    dplyr::summarize(
+      unique_route_count = dplyr::n_distinct(route_id), 


To avoid the notes about undefined variables you need to use .data$route_id here.

ethanplunkett · 2025-01-22T01:02:34Z

R/index_conversions.R

@@ -60,7 +60,7 @@
 #'  space index corresponding to x and y coordinates or row and column indices.}
 #'  }
 #'
-#'  \item{`latlon_to_xy(lat, lon, bf)`}{Returns a two column matrix of the x


This was right as it was bf is an argument to latlon_to_xy()

ethanplunkett · 2025-01-22T01:04:38Z

R/is_between.R

@@ -33,7 +33,7 @@ if (FALSE) {
 #' @param radius A point is considered between two locations if it is within
 #' `radius` meters (along a great circle) of the great circle line between the
 #' locations. `radius` defaults to half the cell size (`mean(res(bf))/2`).
-#' @param n_direction The number of (equally spaced) directional bins to


Thanks for fixing this!

ethanplunkett · 2025-01-22T01:10:42Z

R/route.R

+  points <- points |>
+      dplyr::group_by(route_id) |>
+      dplyr::mutate(
+          date = as.Date(unlist(purrr::accumulate(date, ~ ifelse(.y < .x, .y + lubridate::years(1), .y))))


Currently synthetic routes are allowed to be backwards in time. This assumes forward. We should either change route() to exclude backwards routes or clean things up here. purr is iterating through each pair or adjacent rows so is likely to be slow. It might make sense to fix this in format_trajectory().

ethanplunkett · 2025-01-22T01:14:33Z

R/validate_RouteDataClass.R

+  # check the features required by direct initiation of BirdFlowIntervals class
+  for (name in get_target_columns_BirdFlowIntervals(type='input')){
+    if (!name %in% colnames(birdflow_interval_df)){
+      stop(sprintf(glue::glue("'{name}' is not found in the input dataframe.")))


Drop dependency on glue and use paste() or sprintf()

ethanplunkett · 2025-01-22T01:19:54Z

R/validate_RouteDataClass.R

+
+#' @rdname target_columns
+#' @export
+get_target_columns_Routes <- function(type='input'){


These names feel clunky. I'd be tempted not to export them as I don't think they are for end users, but if we do want to export them I think they should be shorter. get_routes_cols() get_birdflowroutes_cols() ? There's probably something better.

ethanplunkett · 2025-01-22T15:28:01Z

tests/testthat/test-preprocess_species.R

@@ -13,7 +13,22 @@ test_that("preprocess_species runs on test dataset", {
  expect_no_error(a <- preprocess_species("example_data", hdf5 = FALSE))
  expect_no_error(validate_BirdFlow(a, allow_incomplete = TRUE))
  expect_error(validate_BirdFlow(a))
-  expect_true(all((ext(a)[, ] %% xres(a)) < 1e-9))  # Test if origin is at 0, 0


This test is making sure the cells align with the origin and I think is important to keep. It has nothing to do with your code.

I have two thoughts:

I could add code to preprocess species to check if the extent and cell size numbers are close to integers and round them if they are. It wouldn't help when the cell size is floating point (say some sort of spherical coordinates) but would help with all practical applications we've had to date.

We could lower the tolerance on the check to allow the test to pass as is. I can't see the failed test output anymore but if I recall the difference was very small but not quite as small as this test requires.

I think I will leave the current modification as it is -- To suppress the error. I'm not deleting the test -- just using another logic to validate.

Sorry I missed the added lines!

Yours is definitely better.

Your test passes if you are within the tolerance either above or below the exact number, whereas mine only passes if you are above but within the tolerance.
What's weird is when it was failing before it was for a value that was slightly too big given the tolerance so, I think, should have failed either test. Definitely leave yours though, it's both theoretically and empirically working better.

Thanks!

ethanplunkett · 2025-01-22T15:29:59Z

@ethanplunkett Hi Ethan, could you please help review this bug at your convenience?

(About the failed test due to slight discrepancies in cell alignment, that is unrelated to this PR.)
I made a comment here:
https://github.com/birdflow-science/BirdFlowR/pull/204/files#r1925521860
I'll try to fix it in another branch. It's unrelated to your code.

ethanplunkett · 2025-01-22T15:31:53Z

RE: plot_routes() that 31 week stay looks suspicious to me.

chenyangkang · 2025-01-22T15:48:00Z

RE: plot_routes() that 31 week stay looks suspicious to me.

Oh I see, the elapsed_stay is calculated by row_number int he plot_routes function -- Will change it. It should represent 'days' now.

…d_stay_id_with_varied_intervals; remove `glue` package; update some namings

…rrr dependency; fix notes

ethanplunkett · 2025-02-01T11:42:31Z

Looks like you are close!

This is great.

I have one more comment.

Can you add to the top of the news the way this breaks existing code. Something along the lines of:

Breaking Changes:

Old route objects will no longer plot with the updated plot_routes()
Anyone who is using plot_routes() to plot data not generated with route() will have to update their objects. [describe how]

chenyangkang · 2025-02-01T17:11:26Z

@ethanplunkett Done! Could you please help review that next week at your convenience? Ready to be merged.

Still need to do several things:

Go through the historical functions and update the code using the new classes.
Add a plot function for class Routes so that routes in non-BirdFlow coordinates can also be plotted. I think we can use a default CRS and spatiotemporal cell size as argument for that function.

-- These will go to other PRs. I think the current changes are all for this PR.

yc85_illinois_edu and others added 30 commits December 10, 2024 23:13

add the predictive distance metric

c3f1ecb

update for distance_metric

6aa42d4

update latlon_to_xy and xy_to_latlon, so that the bf argument is not …

8b3efd0

…necessary. If bf not provided, use birdflow_crs in the environment. This will make data loading easier without loading the model.

update latlon_to_xy and xy_to_latlon, so that the bf argument is not …

1ea3501

…necessary. If bf not provided, use birdflow_crs in the environment. This will make data loading easier without loading the model.

Changr Rd for index_conversions

86c5873

initial Track and TrackCollection classes

f2901a9

clean code that is no longer used birdflow-science#203

76d0425

reverse a previous modification -- the birdflow_crs is no longer supp…

5f33786

…orted

reverse a previous modification -- the birdflow_crs is no longer supp…

1f29629

…orted

add Tracks, BirdFlowTracks, and related funtions and validations bird…

36975d8

…flow-science#202

add package glue

23d2201

fix typo

17d655c

rename from tracks to routes

24dad6a

remove this as BirdFlowRoutes has new context

b3e1a2c

add BirdFlowIntervals and documents for all RouteDataClass

38aa054

add initial tests for RoutesDataClass -- More to add

d4d3bde

fix bug

39c3257

fix previous errors

b82ea60

update version

d6ce76f

rename the interval sampled n -> max_n

4f599d5

fix some format for lintr; update .Rd in man; update tests

3d66414

small changes

8a52d23

draft of news for v0.1.0.9070

b49e08f

add metadata arguments

9d4ae2e

update seeds as randomly select only one data point for route 001 at …

e14fc35

…timestep 1 can cause inconsistency. So fix the random state.

update dependency

5d5154c

update route function

d1c1e84

update snaps

4b57621

now the dates attributed is always created using get_dates instead of…

1a936e6

… acquiring from BirdFlow models

small fix

5d6ee89

update initial docs for calc_predictive_distance_metric.R

2d88ef1

chenyangkang added 6 commits January 19, 2025 18:45

fix warnings -- docs

de8451d

updates on date/timestep -- timesteps are allowed to have duplicates …

c5fc524

…but not for date; the route generated by `route` function should be sorted

add_stay_id should be based on date, not timestep; and timestep can b…

eaf8aa0

…e duplicated but date should not.

add_stay_id should be based on date, not timestep; and timestep can b…

5a47cdf

…e duplicated but date should not.

add_stay_id should be based on date, not timestep; and timestep can b…

83eaf15

…e duplicated but date should not.

update some variable namings and add docs for variables

cf6a350

ethanplunkett reviewed Jan 22, 2025

View reviewed changes

chenyangkang added 9 commits January 23, 2025 22:59

change Routes related classes to list instead of dataframe; update ad…

24e60a5

…d_stay_id_with_varied_intervals; remove `glue` package; update some namings

change Routes related classes to list instead of dataframe; remove pu…

a253716

…rrr dependency; fix notes

use snap_to_birdflow_inside_of_to_BirdFlowRoutes

f1d0f81

update argument namings

bdcba58

update argument namings

40bacb1

update details based on code review results

cb80cc0

update news

fc47995

add tests and make plot_routes defualt to using week as unit

e44fac9

update docs

430b536

update validation for validate_Routes and update news

4f8a818

ethanplunkett mentioned this pull request Feb 14, 2025

Dynamic mask index conversions #209

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the RoutesDataClass as a generic data format #204

Implement the RoutesDataClass as a generic data format #204

chenyangkang commented Jan 18, 2025 •

edited

Loading

chenyangkang commented Jan 19, 2025

chenyangkang commented Jan 20, 2025

ethanplunkett commented Jan 20, 2025 •

edited

Loading

chenyangkang commented Jan 20, 2025 •

edited

Loading

ethanplunkett commented Jan 20, 2025 via email •

edited

Loading

ethanplunkett left a comment

ethanplunkett Jan 21, 2025

ethanplunkett Jan 21, 2025

ethanplunkett Jan 21, 2025

ethanplunkett Jan 21, 2025

ethanplunkett Jan 22, 2025

ethanplunkett Jan 22, 2025

ethanplunkett Jan 22, 2025

ethanplunkett Jan 22, 2025

ethanplunkett Jan 22, 2025

ethanplunkett Jan 22, 2025

chenyangkang Feb 1, 2025

ethanplunkett Feb 1, 2025

ethanplunkett commented Jan 22, 2025 •

edited

Loading

ethanplunkett commented Jan 22, 2025

chenyangkang commented Jan 22, 2025

ethanplunkett commented Feb 1, 2025

chenyangkang commented Feb 1, 2025

Implement the RoutesDataClass as a generic data format #204

Are you sure you want to change the base?

Implement the RoutesDataClass as a generic data format #204

Conversation

chenyangkang commented Jan 18, 2025 • edited Loading

chenyangkang commented Jan 19, 2025

chenyangkang commented Jan 20, 2025

ethanplunkett commented Jan 20, 2025 • edited Loading

chenyangkang commented Jan 20, 2025 • edited Loading

ethanplunkett commented Jan 20, 2025 via email • edited Loading

ethanplunkett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanplunkett commented Jan 22, 2025 • edited Loading

ethanplunkett commented Jan 22, 2025

chenyangkang commented Jan 22, 2025

ethanplunkett commented Feb 1, 2025

chenyangkang commented Feb 1, 2025

chenyangkang commented Jan 18, 2025 •

edited

Loading

ethanplunkett commented Jan 20, 2025 •

edited

Loading

chenyangkang commented Jan 20, 2025 •

edited

Loading

ethanplunkett commented Jan 20, 2025 via email •

edited

Loading

ethanplunkett commented Jan 22, 2025 •

edited

Loading