Skip to content

Add get_acoustic_deployment_logs() - #517

Open
PietrH wants to merge 169 commits into
mainfrom
254-add-receiver-logs-view
Open

Add get_acoustic_deployment_logs()#517
PietrH wants to merge 169 commits into
mainfrom
254-add-receiver-logs-view

Conversation

@PietrH

@PietrH PietrH commented Mar 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

meeting conclusion:

  • rename and reduce arguments: get_acoustic_deployment_logs(deployment_id, limit = FALSE)
  • change on service side
  • deploy opencpu to production
  • document
  • check if tests need reducing/rewriting
  • rerun tests
  • set minimum httr2 version to v1.0.0 for parallel requests
  • consider http response caching

under construction!

Notes for the reviewer:

get_receiver_logs() was implemented with speed in mind, but compromises were made for maintainability and to not take extra dependencies (yyjsonr, data.table).

  • You will need to install the most recent version of etnservice, this should be handled automatically outside of the testing context.
  • Until etnservice 0.6.0+ is deployed to the production deployment of OpenCPU, you will need to set the ETN_TEST_API environmental variable to the test api url. Contact me if you don't have this value handy. VLIZ has requested we not make it public.
  • deployment_id is required, getting all deployments should not be done likely as it'll take a while.
  • station_name was requested by Lotte
  • I opted to always include argument values in the output tibble.
  • start_date and end_date were implemented to mirror other etn functions, getRad does it better with lubridate support but that is a development for another day.
  • Currently I don't know what fields I'm going to get back from a receiver log, so I implemented name repair to ensure all returned fields are unique and R friendly (no spaces etc.). This also avoids collisions with station_name.
  • Because the returned object is a tidy, (rectangular) tibble, most fields will almost always be empty. This is because a single measurement value can add a whole column even if it's unique within a deployment. Returning as a list is possible, but not currently supported (it would be easy as it could just be an early return). Let me know if you think this is a good idea.
  • I chose not to implement {vcr} HTTP response caching at the moment because as of writing the production {etnservice} deployment is lagging, this way I can ensure that tests will continue to fail until production has caught up.
  • All naming and documentation is open for debate
  • Do you think my tests have enough coverage?
  • Are my failure modes defensive enough?

Before merging

  • switch to production deployment
  • rerun tests

PietrH added 30 commits September 9, 2024 11:14
@PietrH
Init function, unnest log_data
1a3478a
@PietrH
create empty test as placeholder
cbdb56e
@PietrH
make limit optional, check connection
a3d236d
@PietrH
drop duplicate field
fd07ed0
@PietrH
don't disconnect on every function call -> etnservice thing
bfa51cc
@PietrH
document and export
17f4bdb
@PietrH
add dependencies and namespace
9bf0d98
@PietrH
json contains empty strings, parse to NA
a5db0bd
@PietrH
borrow some tests from get_acoustic_detections()
08baa85
@PietrH
colnames to lower, spaces to _, retain units and unit capitalisation
5f93c56
@PietrH
inherit params, filter by date from get_acoustic_detections()
243ed3d
@PietrH
add placeholder test for datetime filtering
00140ac
@PietrH
Improve comments
b7e8a82
@PietrH
filter by receiver_id
94cf3b9
@PietrH
comment out join with acoustic.receivers: too slow
ab2ba1e
@PietrH
filter on deployment_id, most important arg to front
3a4877c
@PietrH
Don't drop any columns, might be different from database datetime
9b444cc
@PietrH
refactor so UTC doesn't become utC
c26aead
@PietrH
adapt test to allow for missing columns
c4b95b2
@PietrH
add placeholder tests
4faa66e
@PietrH
use double join to fetch receiver_id
639ad13
@PietrH
collapse shorter by group: reduce rows into one per deployment, recei…
75221a2 
…ver, record_type, datetime combo
@PietrH
Style and add comments
a5cdee0
@PietrH
Style
40f95b1
@PietrH
Base parsing for column classes
acae096
@PietrH
parse datetime with lubridate
d280590
@PietrH
devtools::document()
6133693
@PietrH
Style
0e6364d
@PietrH
Merge branch 'main' into 254-add-receiver-logs-view
9adaf27
@PietrH
Make deployment id a required field
3934228
@PietrH
PietrH deployed to manual-approval August 21, 2026 10:03 — with GitHub Actions Active
PietrH
PietrH commented Aug 21, 2026
View reviewed changes
Comment thread DESCRIPTION Outdated
@PietrH
PietrH deployed to manual-approval August 21, 2026 11:41 — with GitHub Actions Active
@PietrH
PietrH deployed to manual-approval August 21, 2026 12:19 — with GitHub Actions Active
@PietrH
PietrH marked this pull request as ready for review August 21, 2026 12:46
@PietrH
PietrH requested a review from peterdesmet August 21, 2026 12:47
@PietrH

PietrH commented Aug 21, 2026

Copy link
Copy Markdown
Member Author

Addressed reviewer comments. Ready for review.

@peterdesmet
Update function documentation
ab80cf8 
Ping @PietrH

- Align title with other functions (short + mention "acoustic"). I think logs is pretty well established as a term
- Remove . from title
- Use "log data" in description, not "diagnostics information"
- Don't prepend base::
- No need to define deployment_id, just use inheritParams
- Use "returns" (plural)
@peterdesmet
Update cli warning
9077e11 
Use tidyverse style "Can't find"
peterdesmet
peterdesmet requested changes Sep 2, 2026
View reviewed changes

@peterdesmet peterdesmet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to have this function!

Some minor documentation and class suggestions. See also my commits.

Comment thread R/get_acoustic_deployment_logs.R
#' For example, a deviation in tilt angle may decrease detection capabilities.
#' For some receivers, other information such as water temperature is available.
#'
#' The log data are returned as a tibble with one row per log entry.

@peterdesmet peterdesmet Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new paragraph currently appears under details. If this is not the intent, add @description on line 3

@PietrH PietrH Sep 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was my intent, do you think I should move it?

@peterdesmet peterdesmet Sep 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine under details. I just find it clearer to then add @details

@PietrH PietrH Sep 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite used to the new paragraph = details. But perhaps it's clearer to indicate it explicitly. I think this is best saved for it's own PR, and then applied to all documentation.

Comment thread R/get_acoustic_deployment_logs.R Outdated
Comment thread R/get_acoustic_deployment_logs.R
@peterdesmet

peterdesmet commented Sep 2, 2026

Copy link
Copy Markdown
Member

Note: I did not review the test file.

@peterdesmet
peterdesmet deployed to manual-approval September 2, 2026 15:41 — with GitHub Actions Active
PietrH and others added 4 commits September 3, 2026 10:08
@PietrH
PietrH deployed to manual-approval September 3, 2026 08:27 — with GitHub Actions Active
@PietrH

PietrH commented Sep 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

I'm ready to merge but need reviewer approval! I made all requested changes.

@PietrH
PietrH requested a review from peterdesmet September 3, 2026 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lottepohl lottepohl Awaiting requested review from lottepohl
@peterdesmet peterdesmet Awaiting requested review from peterdesmet

Requested changes must be addressed to merge this pull request.

Assignees

@PietrH PietrH

Labels

None yet

Projects

None yet

Milestone

STRAITS 2026

Development

Successfully merging this pull request may close these issues.

Bug: Use of bare predicate functions in get_acoustic_deployment_logs() Add get_acoustic_deployment_logs() function

3 participants

@PietrH @lottepohl @peterdesmet