You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The reason will be displayed to describe this comment to others. Learn more.
⚠️Code review skipped — your organization has no extra usage available to pay for this review.
If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.
Once extra usage is available, comment @claude review on this pull request, or push a new commit, to trigger a review.
Packaging (R CMD check now returns Status: OK, from 1 NOTE and undeclared
namespaces):
- DESCRIPTION declares what the offline path actually calls: nanoparquet,
terra, sf, TNRS, rWCVP and rWCVPdata in Suggests, stats/tools/utils in
Imports. rWCVPdata is not on CRAN, so Additional_repositories names its
drat. Everything offline stays a Suggest, behind a new nsr_need() guard,
so an install with none of it keeps the API functions working.
- NAMESPACE and man/ regenerated: NSR_local, NSR_local_build,
NSR_local_status, NSR_local_remove and NSR_local_by_region were tagged
@export but never exported, so the local API was unreachable from an
installed package and the cached tests could not have run.
- R/local_globals.R declares the data.table NSE column names.
Resolver bugs:
- The set path never populated country_code/state_code, so every
NSR_local() call of 200 rows or more returned NA for
native_status_country and native_status_state_province. The per-level
codes are now a second and third pass of the same reducer, matching what
the row path does per row; endemism is not applied to them, since Ne/Ie
are claims about a whole range rather than about one level.
- NSR_local_build() never built the WGSRPD raster for a single-system
build, because the only call sat inside the link step. A wcvp-only
cache had WGSRPD opinions and no raster, so coordinate queries silently
matched nothing. It is now built whenever a source publishes against
WGSRPD.
- nsr_consulted_regions() ignored min_overlap while every neighbouring
helper applied it, so a sliver link could turn a result into A or
suppress an Ie. The threshold is now threaded through all four callers.
- nsr_import_wcvp() filtered dist but not the parallel index j, so family,
genus and rank came from unfiltered row positions while every other
column came from the filtered frame - a recycling error, or silently
wrong taxonomy where the lengths happened to divide.
- Extinct WCVP records were read and then ignored, so a taxon that no
longer occurs in a region came back native there. They are now dropped.
Doubtful records stay as "present": too weak for native or introduced,
but dropping them would read as a confident absence.
- isCultivatedNSR was 0 for absence in the set path and NA in the row
path, so the same query changed answer on batch size alone. Both now
say NA. Likewise scope, conflict_type and the sub-polygon counts for
rows with no taxon or no place.
Contract:
- NSR_local() carries user_id through, or generates sequential ids, on
NSR()'s terms, so results join back to their input.
- county_parish now warns that it is not resolved offline instead of
returning a silent NA, and the help says so.
- NSR_local_build() documented a download step it does not have, and
defaulted to two sources that always stop without a local archive. It
now defaults to the one source that can build unattended and the help
states that the archives are not fetched for you.
The Kew source is named wcvp rather than powo (BM): it is the WCVP archive
that is read, and TNRS_local(sources = "wcvp") is what the sibling package calls the same data.
"powo" is accepted as a synonym in sources= and files=. Known cost,
recorded in the design note: native_status_sources no longer matches the
live service's label for this source.
tests/testthat/test-local-nsr-paths.R compares the row and set paths
column for column on tables held in memory, so the 200-row switch cannot
diverge again without a failure. nsr_index_db() was split out of
nsr_local_db() to make that possible without a built cache.
Not changed: Copilot's finding that terra::extract()[, 1] picks up an ID
column. extract() adds ID for a SpatVector, not for the coordinate matrix
passed here; verified against terra 1.9.50, where the call returns the
raster value.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Exported function omits documented companion columns
R/NSR_local_set.R:83
The exported function documents that it returns the same companion columns as NSR_local(), but this constructor omits the per-level status columns (native_status_country and native_status_state_province) and the boolean native_status_conflict (among other service-shaped fields). Consumers cannot use this endpoint as the documented drop-in alternative or distinguish a conflict from conflict_type alone. Either return the advertised schema or document the reduced contract.
The default build could not answer a single query by name. Names always
resolve to GADM through the GNRS backbone, whatever geography a source
publishes against, so a WGSRPD source is unreachable by name until the two
are linked - but link construction was gated on length(systems) > 1, which
a wcvp-only build never satisfies. Making wcvp the default in the previous
commit turned that into the common case. The guard now asks whether
linking is needed at all, not how many systems are present.
Rasterization and link failures were caught by try() and then ignored, so a
cache could be reported as built while being unable to place a coordinate
or reach a WCVP opinion. Both now test for try-error and warn with the
underlying message and the specific consequence, and the link step is
skipped when the raster failed rather than failing again on the same cause.
GADM parent/child containment is now resolved directly (BM's call). gid_1
carries its country in the key ("BRA.25_1" sits in "BRA"), so the relation
is exact and is deliberately kept out of the link table: link rows carry
overlap fractions and are filtered by min_overlap, and a state is a
legitimately tiny share of its country - Distrito Federal is 0.07% of
Brazil and would have been discarded as a sliver. Without this a country
query could not see checklist rows published against that country's states,
which is how VASCAN and Flora do Brasil publish, so native-up propagation,
country coverage and confined-country Ie all failed at country level.
Two things the row/set agreement test caught while wiring that up:
- Only the downward direction belongs here. A first pass added parent
edges too, which made a bare region query silently inherit a country it
was never given; upward inheritance was already handled, where the caller
asks for it, by the ancestor path and by country_keys.
- nsr_confined_ranges() ran before the hierarchy was derived, so
confined-country Ie quietly did nothing. The derivation moved above the
data.table block.
NSR_local_by_region() returns the per-level codes and native_status_conflict
its help advertised, and the help now says plainly that it does not echo the
input columns NSR_local() carries through, since it is given ids and polygon
keys rather than a record.
Three tests cover the hierarchy: a country query seeing the states inside
it, min_overlap leaving the parent/child edges alone, and a range confined
to one country's states resolving to that country.
Not changed, and why:
- "Regenerate NAMESPACE and exported documentation" is stale. All five
exports and all five Rd files are on the branch already.
- terra::extract()[, 1] is correct. extract() adds an ID column for a
SpatVector, not for the coordinate matrix passed here.
- Doubtful WCVP records stay as "present" rather than being dropped.
Dropping them would turn a source's explicit "maybe here" into a
confident absence. Extinct records are filtered, as of the last commit.
Still open: coordinates and division names are unioned when both are given,
so a record whose point and names disagree is answered from both. The
service has no combined mode to follow - NSR() takes names only and
NSR_from_coordinates() takes coordinates only - so this needs a decision
rather than a precedent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
WCVP's `extinct` is a bare 0/1 with no date: checked against the v15
distribution table, whose only columns are plant_locality_id,
plant_name_id, continent_code_l1, continent, region_code_l2, region,
area_code_l3, area, introduced, extinct and location_doubtful. It means
"considered no longer present as of this release" and cannot be compared
against an occurrence's own date.
Whether such a record counts is therefore a property of the question rather
than of the data, so the previous commit's build-time filter was in the
wrong place. The build now keeps the row with an is_extinct flag and
NSR_local(exclude_extinct = TRUE) decides per call (BM): the default
answers about the present day, FALSE models a past distribution.
NSR_local_by_region() takes the same argument. 2,701 of 1,970,252 rows
are affected, over 2,393 taxa, 906 of which have no surviving native
record at all.
Dropping the rows at build time also destroyed evidence the endemism rules
need. A region a taxon has been lost from is still a region it was native
to, so the Ie rule must not fire against it: the answer for a natively
extirpated record is A (gone), never I or Ie (arrived). With the rows
filtered out of the cache entirely, a taxon whose surviving range happened
to be confined would have come back "introduced here (inferred)" for a
record that was natively there - a worse answer than the N it replaced,
and one this filter introduced. The endemism rules now read the taxon's
whole native range, extinct records included, whatever exclude_extinct
says; only the status opinions are filtered.
The set path filters after its join rather than holding a second copy of
the checklist, since the rows removed are 0.14% of it. Caches built before
this default the column to 0, which is true of every row they hold.
Two tests: that the switch moves the answer between A and N for a lost
population, and that t7 (never present) resolves Ie while t6 (present and
lost) resolves A - so the inference machinery is demonstrably live in the
case it must not fire for.
R CMD check: Status OK.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…l them
Two related changes to how NSR_local() decides where a record is.
Coordinates are now opt-in. use_coordinates defaults to FALSE (BM), and
any latitude and longitude present are echoed but not consulted unless it
is set. Placing every record by point is a raster lookup per call, the
most expensive thing in the query path, and it was being paid whether or
not the caller wanted it; and turning coordinates into political divisions
is GVS's job, so doing it here too means two implementations of one lookup
drifting apart. The coordinate path stays available, because consulting
each source in its own geography with no crosswalk is the finer question -
but it is now a choice rather than a default.
When both are given, they are compared rather than pooled. The region keys
from the point and from the names used to be unioned, so a record whose two
statements of place disagreed was answered from evidence for both and
described neither. Coordinates are not made authoritative instead: a
transposed longitude and a mistyped province are equally easy mistakes.
The two are compared in the geography they share, GADM, at each level
separately - so a right country with a wrong state is caught as readily as
a wrong country - and a level only one of them speaks to is not a
disagreement. Where they agree the wider key set answers, as before.
Where they do not, place_conflict is TRUE, native_status is UNK, and
native_status_coordinates and native_status_names carry the two answers for
the caller to judge.
The service has no combined mode to follow here: NSR() takes names and
NSR_from_coordinates() takes coordinates, and the two are separate
endpoints. The existing NSR_from_coordinates() example already renames
country to country_declared so declared and inferred divisions can be
compared, which is the same instinct.
Those columns stay in the output whether or not coordinates are used,
rather than the schema changing with an argument: place_conflict is FALSE
and the two per-basis columns are NA. A message, suppressible with quiet,
says when coordinates were supplied and not used, since occurrence data
usually carries them and their being ignored should not be silent.
nsr_places_agree() and nsr_split_keys() are split out and tested directly,
since nsr_query_regions() needs a built cache and they do not. The cached
reference test asks for coordinates explicitly and now says so; it also
asserts the new default, that the row with coordinates and no division
names has no place at all while the named rows are unchanged.
R CMD check: Status OK.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
⚠️Code review skipped — your organization has no extra usage available to pay for this review.
If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.
Once extra usage is available, comment @claude review on this pull request, or push a new commit, to trigger a review.
…ences
Packaging and build:
- nsr_cache_dir() called tools::R_user_dir(), which arrived in R 4.0.0, while
the package declares R (>= 3.5.0). On older R the local entry points
failed in their default argument before doing any work. It now says what
is missing and how to set it. Inventing a different default there would
have put the cache where GNRS and GVS do not look and given each package
its own copy, which is worse than an error; raising the package minimum
would bind the API functions, which do not need it.
- Adding a source to a cache built before extinct records were retained
failed on the bind: the rows kept from the old checklist have no
is_extinct column, and rbind() sees two schemas. nsr_index_db() already
defaulted the column on read, but that is too late for the rebuild. The
retained rows are normalised first.
- Two warning strings held a real newline where the escape was meant. They
parse, and R CMD check has been passing, but they were not what was
written.
Four more places the row and set paths disagreed, each found by asking the
fixture rather than by reading:
- A matched taxon with no place keys: the row path says "Place not matched
to any region", the set path fell through to the generic absence logic and
said "No comprehensive checklist covers this polygon".
- conflict_type was NA in the set path for any result nothing disagreed
about - a country-only query, or one whose opinions are all contains or
overlaps - where the row path returns the contract value "none".
- Absence was not read against the country a state was given with, although
that country's opinions already feed the answer, so a state with no
coverage of its own inside a comprehensively listed country was UNK in the
row path and A in the set path.
- A country row is now its own container for the confined-range test. A
taxon native both to gadm0:CAN and to a Canadian state had no common
container, so the Ie inference was missed in both paths: gadm0 has no
parent to look up and its links only reach other geographies.
Four tests cover them. NSR_local()'s help listed "four added columns" and
now names what it actually returns.
R CMD check: Status OK.
Not changed, and why:
- terra::extract()[, 1] is correct for the coordinate matrix passed here;
the ID column is a SpatVector behaviour.
- "Regenerate NAMESPACE" remains stale: the exports and Rd files have been
on the branch since the first review pass.
- The GADM hierarchy is deliberately not in the link table. Link rows carry
overlap fractions and are filtered by min_overlap, and a state is a
legitimately tiny share of its country; the containment is exact, so it is
resolved directly instead.
- Doubtful WCVP records stay "present": dropping them would turn a source's
explicit "maybe here" into a confident absence.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The design note took the service's taxonomic rule wholesale, including "with
no opinion for the taxon, higher ranks are consulted". The resolver never
did that, so the note described something the code does not do. The code is
right and the note was wrong (BM).
The rule's two halves are not the same claim. Upward is sound and is
implemented, at build time in nsr_species_of(): a distribution recorded
against an accepted infraspecific taxon is rolled up to its species, because
a subspecies of X occurring natively somewhere means X occurs natively there
- the subspecies is an X. About 60k WCVP rows reach their species this way.
Downward from a higher rank is the ecological fallacy in its division form:
knowing a genus is native to a region tells us nothing about any particular
species in it. A genus native to Quebec means some species of it is native
to Quebec, and the species in hand is as readily the introduced one. Native
is the strongest claim the resolver makes, and making it at genus level would
manufacture it at scale for precisely the taxa the sources are silent about.
So a species with no opinion of its own is UNK, or A where a comprehensive
source covers the place, never native-by-genus; taxon_evaluable separates
them. A genus query is still answered directly from the distributions WCVP
records against genera, which is answering what was asked rather than
inferring downward from it.
This is a permanent divergence from the live service and is recorded as one,
alongside the wcvp/powo source-name difference, for validation against NSR().
The note now says so in the propagation rules, in the resolution semantics,
in the milestone that claimed to implement it, and in a section of its own.
NSR_local()'s help gains a Rank paragraph, since a caller could otherwise
reasonably expect the service's behaviour.
A test pins it: the genus answers when the genus is asked about, the species
does not borrow that answer, and it is not evaluable on the strength of it.
R CMD check: Status OK.
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
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
No description provided.