Replace days with dates, naming the exact dates to fetch - #17
Merged
Conversation
`days` took day-of-month numbers and applied them to every requested
month, which describes a sampling rule rather than a set of dates. Survey
dates are not a rule: they fall on the 3rd in January and the 17th in
February, and no day-number selection expresses that. Matching daily data
to observations was the point of daily support, and `days` could not do
it.
`dates` names the dates instead, so the argument can come straight from
the observations being matched:
accessEnvDat(vars = "SST", dates = unique(observations$date),
bounding_box = bb)
YYYYMMDD strings, YYYY-MM-DD strings, Date objects, and YYYYMMDD numbers
are all accepted. Dates are sorted and deduplicated. A date the calendar
does not have is an error naming it, since dropping it would fetch fewer
days than were asked for and say nothing.
Because `dates` names the time steps, `years` and `months` are no longer
required, and passing them alongside it is an error rather than two
answers to one question with no rule for which wins.
Also fixes a matching bug this exposed. detect_temporal_resolution()
reads daily data from more than one day within a month, so a set of
survey dates - one per month - was indistinguishable from monthly data,
and matchData() would join by month and ignore the day. That is the
opposite of why anyone passes `dates`. accessEnvDat() knows which dataset
it fetched, so it records the step on the result and the detector trusts
that over its heuristics. An explicit temporal_resolution still overrides
both.
One trap worth naming in the error text: c() on a mixture of strings and
Dates converts the Dates to days-since-1970 before this ever sees them.
Being told that 18310 is not a date is unhelpful without the cause.
Verified against the live API: three dates on different days of three
months fetch those days, come back as daily, and match observations on
exactly those dates.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
daystook day-of-month numbers and applied them to every requested month. That describes a sampling rule, not a set of dates. Survey dates are not a rule — they fall on the 3rd in January and the 17th in February, and no day-number selection expresses that.Matching daily data to observations was the point of daily support, and
dayscould not do it.YYYYMMDDstrings,YYYY-MM-DDstrings,Dateobjects, andYYYYMMDDnumbers are all accepted. Dates are sorted and deduplicated. A date the calendar does not have is an error naming it, since dropping it would fetch fewer days than were asked for and say nothing.Because
datesnames the time steps,yearsandmonthsare no longer required, and passing them alongside it is an error rather than two answers to one question with no rule for which wins.A matching bug this exposed
detect_temporal_resolution()reads daily data from more than one day within a month. A set of survey dates — one per month — is indistinguishable from monthly data by that test, somatchData()would join by month and ignore the day. That is the opposite of why anyone passesdates.accessEnvDat()knows which dataset it fetched, so it now records the step on the result and the detector trusts that over its heuristics. An explicittemporal_resolutionstill overrides both.Caught by testing against the live API rather than by reading — the fetch looked right and the resolution was wrong.
One trap named in the error text
c()on a mixture of strings andDates converts the Dates to days-since-1970 before the function sees them. Being told that18310is not a date is unhelpful without the cause, so that case is diagnosed specifically.Verification
Live API: three dates on different days of three months fetch those days, come back as daily, and match observations on exactly those dates.
R CMD check— Status: OK. Full suite green; thedaystest block is replaced by 13datestests covering input forms, sorting, dedupe, invalid dates, thec()coercion trap, the contradictions, and the recorded step.Breaking
daysis removed rather than deprecated — it landed today, is unreleased, and the two arguments mean different things, so an alias would be a rule masquerading as a date list.🤖 Generated with Claude Code