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

added xronos to getter functions #150

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

MartinHinz
Copy link
Contributor

time to give back ;-)

site,xronos,site
,xronos,site_phase
feature,xronos,feature
,xronos,period
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't that fit to c14bazAAR.period, which is defined as

Historico-cultural period of sample context

and usually contains values like "Bronze Age" etc.

,xronos,site_phase
feature,xronos,feature
,xronos,period
,xronos,typochronological_unit
Copy link
Member

Choose a reason for hiding this comment

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

That should fit to c14bazAAR.culture, right?

Copy link
Member

Choose a reason for hiding this comment

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

btw. the variable definition table is available here: https://github.com/ropensci/c14bazAAR/blob/master/data-raw/variable_definition.csv

R/get_xronos.R Outdated
#' @export
get_xronos <- function(db_url = get_db_url("xronos")) {

check_connection_to_url(db_url)
Copy link
Member

Choose a reason for hiding this comment

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

Does this already download the whole database, because db_url points to the endpoint at https://xronos.ch/api/v1/data/? It takes forever. Probably it's sufficient to check if https://www.xronos.ch is up here. Or maybe the API has a more minimal endpoint to check if it's available.

R/get_xronos.R Show resolved Hide resolved
R/get_xronos.R Outdated

xronos_data <- jsonRespParsed %>% tidyr::tibble() %>%
tidyr::unnest_wider(1) %>%
tidyr::unnest_wider(measurement)
Copy link
Member

Choose a reason for hiding this comment

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

These procedures take a significant amount of time and could be avoided with a simple flat file download.... 😅

@joeroe
Copy link
Contributor

joeroe commented Jul 29, 2024

@nevrome @MartinHinz We now finally provide a flat file with all our data (https://xronos.ch/data.csv; see xronos-ch/xronos.rails#115) – perhaps time to revisit this?

@nevrome
Copy link
Member

nevrome commented Aug 1, 2024

Neat! Would be easy to do this now, I guess. Cool that you decided to include the flat-file version 🥳

@nevrome
Copy link
Member

nevrome commented Aug 1, 2024

Probably it would be easier to create a new PR for this. This one is a bit dated...

@MartinHinz
Copy link
Contributor Author

Against your advice, I’ve decided to proceed with this PR and kindly request a new review and potential merge. Given the size of the database, transferring and parsing the 350,000 dates is still taking some time, but we anticipate improvements in the near future.

Here’s the current performance benchmark:

> system.time(get_xronos() -> test2)
   user  system elapsed 
 12.360   0.768  18.113 

Please note, this isn’t strictly a flat file but rather a database dump from a materialized view.

I hope this approach now meets your requirements! Looking forward to your feedback.

@MartinHinz MartinHinz requested a review from nevrome November 29, 2024 14:36
@nevrome nevrome mentioned this pull request Jan 15, 2025
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

Successfully merging this pull request may close these issues.

3 participants