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

Closes #53 #63

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

Closes #53 #63

wants to merge 2 commits into from

Conversation

HughParsonage
Copy link
Collaborator

For some reason

tbl1 <- read_abs(cat_no = "6401.0", tables = 1)
tbl2 <- read_abs(cat_no = "6401.0", tables = 2)
identical(tbl1, tbl2)

regardless of whether I'm using the fst or not (the file downloaded seem to be the same?)

@codecov
Copy link

codecov bot commented Jan 16, 2020

Codecov Report

Merging #63 into master will decrease coverage by 4.84%.
The diff coverage is 51.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
- Coverage   89.37%   84.52%   -4.85%     
==========================================
  Files          16       16              
  Lines         461      504      +43     
==========================================
+ Hits          412      426      +14     
- Misses         49       78      +29
Impacted Files Coverage Δ
R/read_abs_local.R 74.35% <100%> (ø) ⬆️
R/read_abs.R 63.3% <37.2%> (-13.84%) ⬇️
R/fst-utils.R 93.54% <87.5%> (-6.46%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de17abf...12b02b1. Read the comment docs.

@MattCowgill
Copy link
Owner

For some reason

tbl1 <- read_abs(cat_no = "6401.0", tables = 1)
tbl2 <- read_abs(cat_no = "6401.0", tables = 2)
identical(tbl1, tbl2)

regardless of whether I'm using the fst or not (the file downloaded seem to be the same?)

for some weird reason, the first table in 6401.0 is called "Tables 1 and 2". See: https://www.abs.gov.au/AUSSTATS/[email protected]/DetailsPage/6401.0Sep%202019?OpenDocument

@MattCowgill
Copy link
Owner

tables should be a string (I think my current documentation on this gets it wrong). There are some ABS releases (eg. 6345.0: https://www.abs.gov.au/AUSSTATS/[email protected]/DetailsPage/6345.0Sep%202019?OpenDocument) that have table numbers like "2a", "2b" and so on.

@MattCowgill MattCowgill marked this pull request as ready for review January 17, 2020 09:54
@MattCowgill
Copy link
Owner

I accidentally marked this as 'ready for review' just now, sorry

@HughParsonage
Copy link
Collaborator Author

HughParsonage commented Jan 17, 2020 via email

@MattCowgill
Copy link
Owner

Hi @HughParsonage
Sorry, somehow this PR escaped my attention for several months. I am contemplating whether check_local should be TRUE or FALSE by default. I am leaning towards FALSE so that by default, the function fetches the latest data.

@HughParsonage
Copy link
Collaborator Author

Ideally of course, it would be easy: check_local = NA with the logic that if the latest data is the same as the local copy, just use the local copy. But it seems that's basically impossible to do without downloading the data anyway?

@MattCowgill
Copy link
Owner

Yes -- I'm not sure what you had in mind for this step? Sorry, I was under the impression you had a plan for that...

The only two ways I can think of to verify if local data is up to date (or likely to be up to date) are:

  1. Infer likely latest release date from the local file. The local file tells us the periodicity of the data and the latest observation date.
  2. Look up the requested table in the ABS Time Series Database.

Option 1 is fast, but error-prone. Option 2 is slower (though faster than downloading the table(s) ) and requires internet connectivity.

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.

None yet

2 participants