FDM250#254
Conversation
…ply of fertilizer application by interating only once over each fertilizer application
…onia emissions of fertilizer application by interating only once over each fertilizer application
…sition values by requesting more efficient the remote GeoTIFF
…many fields by organizing the calculations in batches
🦋 Changeset detectedLatest commit: 858a3f6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughBatch-fetches GeoTIFF deposition for all fields with an in-memory TIFF cache, processes fields in bounded batches, externalizes deposition to synchronous per-field calculations, consolidates fertilizer supply/emission calculators, updates types/signatures/tests, and adds a 300-field performance test. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant NB as NitrogenBalance (index.ts)
participant Depo as DepositionBatch (supply/deposition.ts)
participant Geo as GeoTIFF Server
Caller->>NB: calculateNitrogenBalance(allFields[], input..., timeFrame, url)
NB->>Depo: calculateAllFieldsNitrogenSupplyByDeposition(fields[], timeFrame, url)
Depo->>Geo: fromUrl(url) via getTiff (cached)
Geo-->>Depo: TIFF image (cached)
Depo->>Depo: compute per-field windows & readRasters (parallel)
Depo-->>NB: depositionByField Map<fieldId,deposition>
NB->>NB: split fields into batches (batchSize)
loop per-batch
NB->>NB: calculateNitrogenBalanceField(field, ..., depositionSupply[fieldId])
end
NB-->>Caller: aggregated farm + fields nitrogen balance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## development #254 +/- ##
===============================================
+ Coverage 92.78% 92.83% +0.04%
===============================================
Files 87 79 -8
Lines 13044 12779 -265
Branches 1294 1252 -42
===============================================
- Hits 12103 11863 -240
+ Misses 939 914 -25
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
coderabbitai
Bot
added
branch:development
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
fdm-calculator/src/balance/nitrogen/input.test.ts (2)
118-128: Likely field property name typo: b_harvesting_id vs b_id_harvestingAcross the codebase (e.g., index test and performance test), harvests use b_id_harvesting. Using b_harvesting_id here can cause type errors or mismatches.
Apply this diff:
- const mockHarvestsData: Harvest[] = [ + const mockHarvestsData: Harvest[] = [ { - b_harvesting_id: "harvest-1", + b_id_harvesting: "harvest-1", b_lu: "cult-1", b_lu_harvest_date: new Date(), b_lu_yield: 1000, b_id_field: "field-1", b_id_farm: b_id_farm, b_principal_id_field: principal_id, b_principal_id_farm: principal_id, }, ]
153-166: Update mock fertilizer application fixtures in input.test.tsThe test currently mocks core fertilizer applications with the old shape (
p_id_fertilizer,p_amount,p_date_applying, etc.), but downstream code and theFertilizerApplicationtype in@svenvw/fdm-corenow expect:
p_app_idp_id_cataloguep_name_nl(optional)p_app_amountp_app_methodp_app_dateWithout this mapping, your balance/supply/emission logic will see
application.p_app_amountasundefinedand default to zero.Please update fdm-calculator/src/balance/nitrogen/input.test.ts:
- const mockFertilizerApplicationsData: FertilizerApplication[] = [ - { - b_fertilizing_id: "fa-1", - p_id_fertilizer: "fert-1", - p_amount: 100, - p_id_field: "field-1", - p_id_farm: b_id_farm, - p_date_applying: new Date(), - p_principal_id_field: principal_id, - p_principal_id_farm: principal_id, - }, - ] + const mockFertilizerApplicationsData: FertilizerApplication[] = [ + { + p_app_id: "fa-1", + p_id_catalogue: "fert-1", + p_name_nl: null, // or a real name if needed + p_app_amount: 100, + p_app_method: "broadcasting", // match one of ApplicationMethods + p_app_date: new Date(), + }, + ]This aligns the fixture with the core type and ensures the calculator pipeline sees the correct properties.
fdm-calculator/src/balance/nitrogen/index.ts (3)
2-2: Incorrect Decimal import will break runtime and instanceof checks
decimal.jsexports a default. The named import{ Decimal }is undefined at runtime. This impacts all usages and theinstanceof Decimalcheck in conversion.Apply this diff:
-import { Decimal } from "decimal.js" +import Decimal from "decimal.js"
155-161: Don’t mutate the shared timeframe; use a field-local copyMutating
timeFrameinsidecalculateNitrogenBalanceFieldleaks into subsequent fields in the loop. Create a localfieldTimeFrameand pass it to downstream calculations.Apply this diff:
- // If timeframe is broader than field existence, shorten it - if (field.b_start?.getTime() > timeFrame.start.getTime()) { - timeFrame.start = field.b_start - } - if (field.b_end?.getTime() < timeFrame.end.getTime()) { - timeFrame.end = field.b_end - } + // Use a field-local timeframe (intersection with input timeframe) + const fieldTimeFrame = { + start: + field.b_start && field.b_start.getTime() > timeFrame.start.getTime() + ? field.b_start + : timeFrame.start, + end: + field.b_end && field.b_end.getTime() < timeFrame.end.getTime() + ? field.b_end + : timeFrame.end, + } @@ - const supply = calculateNitrogenSupply( + const supply = calculateNitrogenSupply( cultivations, fertilizerApplications, soilAnalysis, cultivationDetailsMap, fertilizerDetailsMap, depositionSupply, - timeFrame, + fieldTimeFrame, ) @@ - const target = calculateTargetForNitrogenBalance( + const target = calculateTargetForNitrogenBalance( cultivations, soilAnalysis, cultivationDetailsMap, - timeFrame, + fieldTimeFrame, )Also applies to: 163-172, 191-196
226-236: Normalize Decimal construction and use ammonia.total consistently at farm level
- Use
new Decimal(0)consistently (now that the default import is fixed) for clarity.- Field-level balance uses
emission.ammonia.total; the farm aggregation should mirror that to avoid pulling in any other emission components inadvertently.Apply this diff:
- const totalFarmArea = fields.reduce( - (sum, field) => sum.add(new Decimal(field.field.b_area ?? 0)), - Decimal(0), - ) + const totalFarmArea = fields.reduce( + (sum, field) => sum.add(new Decimal(field.field.b_area ?? 0)), + new Decimal(0), + ) - let totalFarmSupply = Decimal(0) - let totalFarmRemoval = Decimal(0) - let totalFarmEmission = Decimal(0) - let totalFarmTarget = Decimal(0) + let totalFarmSupply = new Decimal(0) + let totalFarmRemoval = new Decimal(0) + let totalFarmEmission = new Decimal(0) + let totalFarmTarget = new Decimal(0) @@ - totalFarmEmission = totalFarmEmission.add( - fieldBalance.emission.total.times(fieldArea), - ) + totalFarmEmission = totalFarmEmission.add( + fieldBalance.emission.ammonia.total.times(fieldArea), + ) @@ - const avgFarmEmission = totalFarmArea.isZero() + const avgFarmEmission = totalFarmArea.isZero() ? Decimal(0) : totalFarmEmission.dividedBy(totalFarmArea)Note: After fixing the Decimal import, also change the ternary
Decimal(0)tonew Decimal(0)if you prefer consistency across the file.Also applies to: 251-259, 265-275
🧹 Nitpick comments (16)
.changeset/breezy-tigers-heal.md (1)
5-5: LGTM; small style nit.Consider a minor wording tweak and add a period for consistency with other notes.
-Prevent overwhelming the nitrogen balance calculation with many fields by organizing the calculations in batches +Prevent overwhelming the nitrogen balance calculation with many fields by organizing calculations into batches.fdm-calculator/src/balance/nitrogen/types.d.ts (3)
441-441: Typo in comment: “emmission” → “emission”.Public JSDoc should be polished.
- | "p_ef_nh3" // Ammonia emmission factor + | "p_ef_nh3" // Ammonia emission factor
70-75: Typo in comment: “fertilzer” → “fertilizer”.Minor doc polish.
- * A detailed list of individual other fertilzer applications. + * A detailed list of individual other fertilizer applications.
520-531: Parity gap: numeric ammonia emission type lacks compost/other sections.NitrogenEmissionAmmoniaFertilizers includes compost and other, but the numeric counterpart omits them. Consider aligning the numeric type for consistency and easier transformations.
export type NitrogenEmissionAmmoniaFertilizersNumeric = { total: number mineral: { total: number applications: { id: string; value: number }[] } manure: { total: number applications: { id: string; value: number }[] } + compost: { + total: number + applications: { id: string; value: number }[] + } + other: { + total: number + applications: { id: string; value: number }[] + } }fdm-calculator/src/balance/nitrogen/supply/fertilizers.test.ts (1)
39-51: Zeroing new FertilizerDetail fields is fine, but consider adding one non-zero caseSetting p_no3_rt, p_nh4_rt, p_s_rt, p_ef_nh3 to 0 keeps behavior unchanged. To guard against future logic that might use these fields, consider a follow-up test with one non-zero value (e.g., p_no3_rt) to ensure it does not unintentionally affect nitrogen supply totals.
Also applies to: 53-63, 65-75, 77-88
fdm-calculator/src/balance/nitrogen/supply/fertilizers.ts (1)
41-48: Nit: correct unit comments to avoid confusionThe comments currently mention “kg N / kg” and “g N” conversion inconsistently. After the refactor above, adjust doc comments to reflect: applicationValue is kg N derived from kg fertilizer × g N/kg ÷ 1000.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts (2)
101-112: Unit comments say “g”, but numbers (and formulas) imply kg application amountsSeveral comments label p_app_amount as grams, but expected results treat them as kilograms (e.g., 2000 × 20 g/kg × 0.68 / 1000 = 27.2 kg N). To avoid confusion, update comments to “kg” or clarify unit assumptions.
Example fix:
- p_app_amount: 2000, // g + p_app_amount: 2000, // kgAlso applies to: 133-144, 159-170, 186-197, 213-224, 241-269, 289-301, 313-334, 351-373, 391-401, 412-423, 434-445, 456-467, 478-489, 500-511, 522-533, 544-555, 566-577, 588-599, 610-621, 632-643, 654-665, 676-687, 704-715, 732-743, 760-771, 783-794, 806-817
2-2: Remove unused importDecimal is imported but not used in this test file.
Apply this diff:
-import Decimal from "decimal.js"fdm-calculator/src/balance/nitrogen/performance.test.ts (3)
10-11: Remove unused importsDecimal and SoilAnalysisPicked are imported but not used.
Apply this diff:
-import type { - CultivationDetail, - FertilizerDetail, - FieldInput, - NitrogenBalanceInput, - SoilAnalysisPicked, -} from "./types" -import { Decimal } from "decimal.js" +import type { + CultivationDetail, + FertilizerDetail, + FieldInput, + NitrogenBalanceInput, +} from "./types"
165-169: Make batchSize configurable to aid tuning across environmentsInstead of editing source to tune batch size, consider reading from an env var or test parameter. This lets CI and local runs select different values without code changes.
If desired, I can submit a small change to read batchSize from process.env.NB_BATCH_SIZE with a default.
Also applies to: 170-194
63-77: Minor: randomization adds noise to perf measurementsRandomizing inputs is fine here, but it introduces variance. If you start tracking trends over time, consider seeding or fixed values to reduce run-to-run variability.
Also applies to: 110-121, 148-156
fdm-calculator/src/balance/nitrogen/supply/deposition.ts (1)
92-127: Optional: Limit readRasters concurrency and de-duplicate windowsFor hundreds of fields,
Promise.allissues many range requests concurrently. Consider:
- Limiting concurrency (e.g., p-limit at ~32–64).
- De-duplicating identical windows (many centroids map to the same pixel); compute window keys like
${xPx},${yPx}and reuse a single raster read per pixel.I can provide a concrete refactor if you want to go this route.
fdm-calculator/src/balance/nitrogen/supply/deposition.test.ts (2)
39-81: Add a zero-day timeframe test to lock intended semanticsGiven the per-field fraction change, consider asserting that start === end yields a 1/365 fraction (single day), not zero.
Proposed test to add:
it("treats zero-day timeframe as a single day", async () => { const field: FieldInput = { field: { b_centroid: [5.0, 52.0], b_area: 100000, b_id: "test_field_zero_day", b_start: new Date("2025-01-01"), b_end: new Date("2025-12-31"), }, cultivations: [], harvests: [], soilAnalyses: [], fertilizerApplications: [], } const timeFrame = { start: new Date("2025-06-01"), end: new Date("2025-06-01") } const resultMap = await calculateAllFieldsNitrogenSupplyByDeposition( [field], timeFrame, fdmPublicDataUrl, ) const result = resultMap.get("test_field_zero_day") expect(result).toBeDefined() // Pixel value at [5.0, 52.0] ~ 19.572 → single day ≈ 19.572 / 365 expect(result!.total.toNumber()).toBeCloseTo(19.572 / 365, 4) })
1-8: Avoid network reliance in unit tests (optional)These tests hit GCS. For speed and determinism, mock geotiff.js or use a small local fixture TIFF. Keep one integration test under a separate, opt-in suite if needed.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (1)
225-266: Flatten else branches (noUselessElse) for readabilityEach branch returns early. You can drop the
elseblocks to satisfy the linter and reduce nesting.Apply this diff:
- if (landType === "grassland") { + if (landType === "grassland") { ... - } else if (landType === "cropland") { + return new Decimal(0) + } + + if (landType === "cropland") { ... - } else { - // Bare soil + return new Decimal(0) + } + + // Bare soil if (p_app_method === "broadcasting") { return new Decimal(0.69) } ... - } + return new Decimal(0)fdm-calculator/src/balance/nitrogen/index.ts (1)
297-325: Decimal-number conversion: small consistency improvementThis utility relies on
instanceof Decimal. Once the import is fixed, consider usingnew Decimal(0)style consistently and, optionally, making rounding strategy explicit (e.g.,round(0)with mode) to avoid unexpected rounding at .5 boundaries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (34)
.changeset/breezy-tigers-heal.md(1 hunks).changeset/slimy-geckos-invite.md(1 hunks).changeset/strong-walls-bake.md(1 hunks).changeset/ten-garlics-marry.md(1 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts(1 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.test.ts(0 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts(0 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/index.ts(0 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.test.ts(0 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts(0 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.test.ts(0 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts(0 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.test.ts(0 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts(0 hunks)fdm-calculator/src/balance/nitrogen/index.ts(6 hunks)fdm-calculator/src/balance/nitrogen/input.test.ts(5 hunks)fdm-calculator/src/balance/nitrogen/input.ts(0 hunks)fdm-calculator/src/balance/nitrogen/performance.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/supply/deposition.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/supply/deposition.ts(1 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers.test.ts(6 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers.ts(1 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/compost.test.ts(0 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/compost.ts(0 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/index.ts(0 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/manure.test.ts(0 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/manure.ts(0 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/mineral.test.ts(0 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/mineral.ts(0 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.test.ts(0 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.ts(0 hunks)fdm-calculator/src/balance/nitrogen/supply/index.ts(2 hunks)fdm-calculator/src/balance/nitrogen/types.d.ts(1 hunks)
💤 Files with no reviewable changes (19)
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/compost.test.ts
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/manure.test.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.test.ts
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/mineral.test.ts
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.test.ts
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/mineral.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/compost.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts
- fdm-calculator/src/balance/nitrogen/input.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.test.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/index.ts
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/index.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.test.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.test.ts
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/manure.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: SvenVw
PR: SvenVw/fdm#0
File: :0-0
Timestamp: 2025-08-13T10:33:05.313Z
Learning: In the fdm project, fdm-calculator integration for new features like b_lu_variety is handled in separate updates from the core data model changes. When fdm-core functions are updated to support new fields, fdm-calculator can consume these enhanced APIs without requiring changes in the same PR that introduces the core functionality.
📚 Learning: 2025-02-14T09:56:37.606Z
Learnt from: SvenVw
PR: SvenVw/fdm#75
File: fdm-app/app/routes/farm.$b_id_farm.field.$b_id.fertilizer.tsx:68-71
Timestamp: 2025-02-14T09:56:37.606Z
Learning: The `calculateDose` function in `svenvw/fdm-calculator` is a synchronous function that includes built-in validation for negative application amounts and nutrient rates.
Applied to files:
.changeset/slimy-geckos-invite.md
📚 Learning: 2025-08-13T10:33:05.313Z
Learnt from: SvenVw
PR: SvenVw/fdm#0
File: :0-0
Timestamp: 2025-08-13T10:33:05.313Z
Learning: In the fdm project, fdm-calculator integration for new features like b_lu_variety is handled in separate updates from the core data model changes. When fdm-core functions are updated to support new fields, fdm-calculator can consume these enhanced APIs without requiring changes in the same PR that introduces the core functionality.
Applied to files:
.changeset/breezy-tigers-heal.md
📚 Learning: 2025-08-11T11:55:26.053Z
Learnt from: SvenVw
PR: SvenVw/fdm#233
File: fdm-app/app/integrations/nmi.ts:54-0
Timestamp: 2025-08-11T11:55:26.053Z
Learning: The NMI API Estimates endpoint (`https://api.nmi-agro.nl/estimates`) always returns the fields `b_gwl_ghg`, `b_gwl_glg`, and `cultivations` according to its specification. These fields should be kept as required (not optional) in the TypeScript return type and Zod validation schema in `fdm-app/app/integrations/nmi.ts`.
Applied to files:
fdm-calculator/src/balance/nitrogen/input.test.ts
📚 Learning: 2024-11-27T12:15:36.425Z
Learnt from: SvenVw
PR: SvenVw/fdm#9
File: fdm-data/src/cultivations/index.test.ts:57-59
Timestamp: 2024-11-27T12:15:36.425Z
Learning: In `fdm-data/src/cultivations/index.test.ts`, the `fdm` object created by `drizzle` does not have an `.end()` method. Cleanup code should not attempt to call `fdm.end();`.
Applied to files:
fdm-calculator/src/balance/nitrogen/input.test.ts
📚 Learning: 2025-03-06T15:23:48.352Z
Learnt from: SvenVw
PR: SvenVw/fdm#95
File: fdm-core/src/catalogues.ts:134-170
Timestamp: 2025-03-06T15:23:48.352Z
Learning: When writing tests for fdm-core, avoid using Vitest's `vi` mocking utilities and prefer manual JavaScript mocks.
Applied to files:
fdm-calculator/src/balance/nitrogen/input.test.ts
📚 Learning: 2025-01-31T15:05:14.310Z
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:05:14.310Z
Learning: The `updateField` function in fdm-core has optional parameters after `fdm` and `b_id`. The TypeScript definitions might show 8 required parameters due to a potential version mismatch.
Applied to files:
fdm-calculator/src/balance/nitrogen/input.test.ts
📚 Learning: 2025-08-14T14:31:55.362Z
Learnt from: SvenVw
PR: SvenVw/fdm#236
File: fdm-calculator/src/balance/nitrogen/index.ts:173-0
Timestamp: 2025-08-14T14:31:55.362Z
Learning: In nitrogen balance calculations for agricultural systems, the balance should only include ammonia emissions (emission.ammonia.total) and should not include nitrate leaching from the emission calculation. The nitrate component (emission.nitrate) should be excluded from the balance formula.
Applied to files:
fdm-calculator/src/balance/nitrogen/index.ts
🧬 Code Graph Analysis (8)
fdm-calculator/src/balance/nitrogen/supply/fertilizers.ts (1)
fdm-calculator/src/balance/nitrogen/types.d.ts (2)
FertilizerDetail(434-444)NitrogenSupplyFertilizers(16-77)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (3)
CultivationDetail(420-429)FertilizerDetail(434-444)FieldInput(395-415)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (1)
calculateNitrogenEmissionViaAmmoniaByFertilizers(21-132)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (3)
fdm-core/src/db/schema.ts (2)
cultivations(289-301)fertilizerApplication(180-197)fdm-calculator/src/balance/nitrogen/types.d.ts (4)
FieldInput(395-415)CultivationDetail(420-429)FertilizerDetail(434-444)NitrogenEmissionAmmoniaFertilizers(190-251)fdm-calculator/src/balance/nitrogen/emission/ammonia/index.ts (1)
calculateNitrogenEmissionViaAmmonia(21-51)
fdm-calculator/src/balance/nitrogen/performance.test.ts (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (2)
FertilizerDetail(434-444)CultivationDetail(420-429)fdm-calculator/src/balance/nitrogen/index.test.ts (1)
mockNitrogenBalanceInput(21-116)
fdm-calculator/src/balance/nitrogen/supply/deposition.ts (1)
fdm-calculator/src/balance/nitrogen/types.d.ts (3)
FieldInput(395-415)NitrogenBalanceInput(449-457)NitrogenSupply(114-135)
fdm-calculator/src/balance/nitrogen/supply/deposition.test.ts (3)
fdm-calculator/src/balance/nitrogen/index.ts (1)
getFdmPublicDataUrl(424-426)fdm-calculator/src/balance/nitrogen/types.d.ts (2)
FieldInput(395-415)NitrogenBalanceInput(449-457)fdm-calculator/src/balance/nitrogen/supply/deposition.ts (1)
calculateAllFieldsNitrogenSupplyByDeposition(65-135)
fdm-calculator/src/balance/nitrogen/supply/index.ts (1)
fdm-calculator/src/balance/nitrogen/types.d.ts (6)
FieldInput(395-415)SoilAnalysisPicked(381-390)CultivationDetail(420-429)FertilizerDetail(434-444)NitrogenSupply(114-135)NitrogenBalanceInput(449-457)
fdm-calculator/src/balance/nitrogen/index.ts (3)
fdm-calculator/src/balance/nitrogen/supply/deposition.ts (1)
calculateAllFieldsNitrogenSupplyByDeposition(65-135)fdm-calculator/src/balance/nitrogen/types.d.ts (2)
NitrogenBalanceField(323-348)FieldInput(395-415)fdm-calculator/src/balance/nitrogen/supply/index.ts (1)
calculateNitrogenSupply(29-79)
🪛 LanguageTool
.changeset/slimy-geckos-invite.md
[grammar] ~5-~5: Ensure spelling is correct
Context: ...for supply of fertilizer application by interating only once over each fertilizer applicat...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
.changeset/ten-garlics-marry.md
[grammar] ~5-~5: Ensure spelling is correct
Context: ... emissions of fertilizer application by interating only once over each fertilizer applicat...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
.changeset/strong-walls-bake.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...nce for retrieving deposition values by requesting more efficient the remote GeoTIFF
(QB_NEW_EN)
[grammar] ~5-~5: There might be a mistake here.
Context: ...ion values by requesting more efficient the remote GeoTIFF
(QB_NEW_EN)
🪛 Biome (2.1.2)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts
[error] 243-290: This else clause can be omitted because previous branches break early.
Safe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 266-290: This else clause can be omitted because previous branches break early.
(lint/style/noUselessElse)
🔇 Additional comments (10)
fdm-calculator/src/balance/nitrogen/supply/fertilizers.test.ts (2)
3-4: Imports realigned correctlySwitching to local module paths for types and the aggregator looks good and matches the refactor.
95-100: Expected totals match unit conversion (kg input, g/kg content)The totals (31, 20, 7.5, 2.5, 1) correctly reflect p_app_amount in kg and p_n_rt in g/kg.
fdm-calculator/src/balance/nitrogen/supply/fertilizers.ts (1)
70-88: Default branch acts as “other”; consider explicit typing or guardThe switch default routes all unknown p_type values into the other bucket. If p_type is a closed set, consider typing it as a union and throwing on unexpected values to surface data issues earlier. If an open set is intended, keep as-is.
Would you like me to tighten the FertilizerDetail.p_type type to a union and add a guard?
fdm-calculator/src/balance/nitrogen/input.test.ts (1)
82-100: b_centroid shape differs from other tests (GeoJSON vs tuple)Here b_centroid is a GeoJSON Point object, while other tests use a [lon, lat] tuple. Ensure Field’s b_centroid type accepts both or normalize representation to avoid downstream consumers tripping on shape differences.
Would you like me to normalize test fixtures to a consistent b_centroid shape?
Also applies to: 290-301
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts (2)
1-10: Strong coverage and realistic scenariosGreat consolidation test suite covering per-type paths, inhibitors, land-use detection, and warnings. This will catch regressions introduced by the refactor.
688-701: Good negative-path assertions for unsupported methodsThe exact console.warn assertions for grassland/cropland/bare soil are precise and helpful.
Also applies to: 716-729, 744-758
fdm-calculator/src/balance/nitrogen/supply/deposition.test.ts (1)
9-37: Tests look good for the new batch APIThe switch to Map-based assertions per field ID is clear and aligns with the new batch function. Values and timeframe scaling checks make sense.
fdm-calculator/src/balance/nitrogen/supply/index.ts (1)
29-37: Solid refactor to accept precomputed deposition and return sync supplyDecoupling deposition I/O from per-field supply is the right direction for performance and composition. The API shape is cleaner and easier to test.
Also applies to: 60-73
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (1)
35-45: Ignore obsolete p_id_catalogue presence concernAll
FertilizerApplicationobjects in our pipeline include a non‐nullablep_id_catalogue:
- The
FertilizerApplicationinterface infdm-core/src/fertilizer.d.tsexplicitly declaresp_id_catalogue: string.- Upstream data mappings in both the cultivation and dosing modules populate that field before handing data off to the calculator.
There’s no scenario where
application.p_id_cataloguewould be undefined, so the lookup inemission/ammonia/fertilizers.tsis safe.Likely an incorrect or invalid review comment.
fdm-calculator/src/balance/nitrogen/index.ts (1)
56-63: Great move: batch deposition fetch with a single GeoTIFFPrefetching deposition once and reusing it across fields is exactly the optimization needed to avoid repeated TIFF downloads. Nice.
…ons are available
…and fix zero-day handling
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fdm-data/src/fertilizers/hash.test.ts (1)
61-66: Update expected hash to match the new input shape (pipeline failure).The new
p_ef_nh3field modifies the hash; tests now fail with:
Line 65: AssertionError: expected '08585267' to be '5a405bbe'.Update the expected value.
Apply this diff:
- expect(hash).toBe("5a405bbe") + expect(hash).toBe("08585267")
♻️ Duplicate comments (2)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (2)
35-45: Skip malformed applications with missing dates at aggregation levelShort-circuit early to keep the flow resilient and avoid invoking downstream logic with undefined dates.
const aggregatedEmissions = fertilizerApplications.reduce( (acc, application) => { + if (!application.p_app_date) { + console.warn( + `Missing application date for ${application.p_name_nl} (${application.p_app_id}); skipping application`, + ) + return acc + } const fertilizerDetail = fertilizerDetailsMap.get( application.p_id_catalogue, )
181-199: Guard against missing application date before getTime()p_app_date can be null from DB; calling getTime() will throw. Return 0 for manure factor when date is missing.
Apply:
function determineManureAmmoniaEmissionFactor( fertilizerApplication: FertilizerApplication, cultivations: FieldInput["cultivations"], cultivationDetails: Map<string, CultivationDetail>, ) { const p_app_name = fertilizerApplication.p_name_nl const p_id = fertilizerApplication.p_id const p_app_date = fertilizerApplication.p_app_date const p_app_method = fertilizerApplication.p_app_method + if (!p_app_date) { + console.warn( + `Missing application date for ${p_app_name} (${p_id}); returning 0 emission factor`, + ) + return new Decimal(0) + }
🧹 Nitpick comments (10)
.changeset/major-donkeys-rest.md (1)
5-5: Grammar nit: add article for clarity and consider stating the expected range.Recommend “fraction instead of a percentage” and optionally note the 0–1 range to prevent ambiguity in downstream usage.
Apply this diff:
-Fixes unit of `p_ef_nh3` to be a fraction instead of percentage +Fixes unit of `p_ef_nh3` to be a fraction instead of a percentage (value in the 0–1 range).changeset/lucky-owls-help.md (1)
5-5: Clarify units in the description.Small improvement: mention that
p_ef_nh3values are fractions (0–1), not percentages, to align with the related patch note and avoid confusion.Suggested tweak:
-Add `p_ef_nh3` to synced values for `baat` catalogue +Add `p_ef_nh3` (fraction 0–1) to synced values for `baat` cataloguefdm-data/src/fertilizers/hash.test.ts (1)
5-67: Add a focused test to assert that changing p_ef_nh3 changes the hash.This guards against regressions where the new field is accidentally ignored by the hashing logic.
Add this test (place it after the first test):
it("should generate different hashes when p_ef_nh3 changes", async () => { const base: CatalogueFertilizerItem = { p_id_catalogue: "test-id-ef", p_source: "test-source", p_name_nl: "Test Fertilizer EF", p_name_en: "Test Fertilizer (EN)", p_description: "EF test", p_ef_nh3: 0.05, p_dm: 37, p_density: 20, p_om: 20, p_a: 30, p_hc: 40, p_eom: 50, p_eoc: 60, p_c_rt: 70, p_c_of: 80, p_c_if: 90, p_c_fr: 100, p_cn_of: 110, p_n_rt: 120, p_n_if: 130, p_n_of: 140, p_n_wc: 150, p_no3_rt: 150, p_nh4_rt: 150, p_p_rt: 160, p_k_rt: 170, p_mg_rt: 180, p_ca_rt: 190, p_ne: 200, p_s_rt: 210, p_s_wc: 220, p_cu_rt: 230, p_zn_rt: 240, p_na_rt: 250, p_si_rt: 260, p_b_rt: 270, p_mn_rt: 280, p_ni_rt: 290, p_fe_rt: 300, p_mo_rt: 310, p_co_rt: 320, p_as_rt: 330, p_cd_rt: 340, p_cr_rt: 350, p_cr_vi: 360, p_pb_rt: 370, p_hg_rt: 380, p_cl_rt: 390, p_type_manure: true, p_type_mineral: false, p_type_compost: false, } const hash1 = await hashFertilizer(base) const hash2 = await hashFertilizer({ ...base, p_ef_nh3: 0.07 }) expect(hash1).not.toBe(hash2) })fdm-data/src/fertilizers/catalogues/baat.ts (1)
33-34: Mapping LGTM; consider a defensive sanity check on units.Given the recent unit switch to fractions, a lightweight safeguard can help catch stale data (e.g., values > 1) early.
Apply this diff to add a sanity clamp/warn without breaking consumers:
- p_ef_nh3: - fertilizer.p_ef_nh3 === undefined ? null : fertilizer.p_ef_nh3, + p_ef_nh3: + fertilizer.p_ef_nh3 === undefined + ? null + : fertilizer.p_ef_nh3,Optionally, add a non-throwing warning just after item creation (kept minimal to avoid noisy logs in production):
if (item.p_ef_nh3 != null && item.p_ef_nh3 > 1) { // console.warn(`[baat] p_ef_nh3 > 1 for ${item.p_id_catalogue}: ${item.p_ef_nh3}`) }If you prefer not to log here, consider a small data-validation test that asserts all
p_ef_nh3inbaat.jsonare <= 1.fdm-calculator/src/balance/nitrogen/performance.test.ts (2)
40-59: Use canonical cropland/grassland values for b_lu_croprotationThe manure emission logic checks for "crop" or "grass". Using "maize"/"wheat" won’t classify land type, defaulting to “bare soil.” For realistic paths and stable perf, set these to "crop" (or "grass" when intended).
Apply:
- b_lu_croprotation: "maize", + b_lu_croprotation: "crop", @@ - b_lu_croprotation: "wheat", + b_lu_croprotation: "crop",
122-135: Optional: provide an application method to avoid 0-factor manure pathsWithout p_app_method, manure applications get a 0 emission factor. For a more representative perf mix, set a method (e.g., "broadcasting").
{ p_app_id: `fa-${fieldId}-1`, // Randomly pick one of the available fertilizer catalogue IDs p_id_catalogue: fertilizerDetails[ Math.floor(Math.random() * fertilizerDetails.length) ].p_id_catalogue, p_app_amount: Math.floor(Math.random() * 500 + 100), // 100-600 b_id: fieldId, b_id_farm: "test-farm", p_app_date: new Date(2023, 4, 1), + p_app_method: "broadcasting", },fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (2)
242-289: Remove unnecessary else blocks after returns (style, no behavior change)This matches the linter hint (noUselessElse) and reduces nesting.
- } else if (landType === "cropland") { + } + if (landType === "cropland") { if (p_app_method === "broadcasting") { return new Decimal(0.69) } @@ - } else { - // Bare soil + } + // Bare soil if (p_app_method === "broadcasting") { return new Decimal(0.69) }
148-165: Clarify units and clamp determineMineralAmmoniaEmissionFactor to [0..1]Given p_ef_nh3 is a fraction in data, keep units consistent by clamping this computed factor to [0..1] too.
function determineMineralAmmoniaEmissionFactor( fertilizerDetail: FertilizerDetail, ): Decimal { @@ - return a.add(b).add(c) + const factor = a.add(b).add(c) + if (factor.lt(0)) return new Decimal(0) + if (factor.gt(1)) return new Decimal(1) + return factor }fdm-calculator/src/balance/nitrogen/input.test.ts (1)
155-161: Align fertilizer IDs between applications and details to catch real joinsApplications use p_id_catalogue: "fert-1" while details define "fert-cat-1". This test passes but hides join issues. Better to match IDs so end-to-end checks can be added later without surprises.
- { - p_app_id: "fa-1", - p_id_catalogue: "fert-1", + { + p_app_id: "fa-1", + p_id_catalogue: "fert-1", p_name_nl: 'test-product', p_app_amount: 100, p_app_method: "broadcasting", // match one of ApplicationMethods p_app_date: new Date(), },- { - p_id_catalogue: "fert-cat-1", + { + p_id_catalogue: "fert-1", p_n_rt: 5, p_type: "manure", p_no3_rt: 1, p_nh4_rt: 2, p_s_rt: 0, p_ef_nh3: 0.1, },Also applies to: 164-173
fdm-calculator/src/balance/nitrogen/index.ts (1)
235-238: Unify Decimal instantiation styleYou mix
new Decimal(0)andDecimal(0). For consistency, prefernew Decimal(0)everywhere.- const totalFarmArea = fields.reduce( - (sum, field) => sum.add(new Decimal(field.field.b_area ?? 0)), - Decimal(0), - ) + const totalFarmArea = fields.reduce( + (sum, field) => sum.add(new Decimal(field.field.b_area ?? 0)), + new Decimal(0), + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.changeset/lucky-owls-help.md(1 hunks).changeset/major-donkeys-rest.md(1 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts(1 hunks)fdm-calculator/src/balance/nitrogen/index.ts(11 hunks)fdm-calculator/src/balance/nitrogen/input.test.ts(4 hunks)fdm-calculator/src/balance/nitrogen/performance.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/supply/deposition.test.ts(1 hunks)fdm-data/src/fertilizers/catalogues/baat.json(19 hunks)fdm-data/src/fertilizers/catalogues/baat.ts(1 hunks)fdm-data/src/fertilizers/d.ts(1 hunks)fdm-data/src/fertilizers/hash.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- fdm-calculator/src/balance/nitrogen/supply/deposition.test.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: SvenVw
PR: SvenVw/fdm#0
File: :0-0
Timestamp: 2025-08-13T10:33:05.313Z
Learning: In the fdm project, fdm-calculator integration for new features like b_lu_variety is handled in separate updates from the core data model changes. When fdm-core functions are updated to support new fields, fdm-calculator can consume these enhanced APIs without requiring changes in the same PR that introduces the core functionality.
📚 Learning: 2024-11-27T12:15:36.425Z
Learnt from: SvenVw
PR: SvenVw/fdm#9
File: fdm-data/src/cultivations/index.test.ts:57-59
Timestamp: 2024-11-27T12:15:36.425Z
Learning: In `fdm-data/src/cultivations/index.test.ts`, the `fdm` object created by `drizzle` does not have an `.end()` method. Cleanup code should not attempt to call `fdm.end();`.
Applied to files:
fdm-calculator/src/balance/nitrogen/input.test.ts
📚 Learning: 2025-03-06T15:23:48.352Z
Learnt from: SvenVw
PR: SvenVw/fdm#95
File: fdm-core/src/catalogues.ts:134-170
Timestamp: 2025-03-06T15:23:48.352Z
Learning: When writing tests for fdm-core, avoid using Vitest's `vi` mocking utilities and prefer manual JavaScript mocks.
Applied to files:
fdm-calculator/src/balance/nitrogen/input.test.ts
📚 Learning: 2025-01-31T15:05:14.310Z
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:05:14.310Z
Learning: The `updateField` function in fdm-core has optional parameters after `fdm` and `b_id`. The TypeScript definitions might show 8 required parameters due to a potential version mismatch.
Applied to files:
fdm-calculator/src/balance/nitrogen/input.test.ts
📚 Learning: 2025-08-14T14:31:55.362Z
Learnt from: SvenVw
PR: SvenVw/fdm#236
File: fdm-calculator/src/balance/nitrogen/index.ts:173-0
Timestamp: 2025-08-14T14:31:55.362Z
Learning: In nitrogen balance calculations for agricultural systems, the balance should only include ammonia emissions (emission.ammonia.total) and should not include nitrate leaching from the emission calculation. The nitrate component (emission.nitrate) should be excluded from the balance formula.
Applied to files:
fdm-calculator/src/balance/nitrogen/index.ts
🧬 Code Graph Analysis (4)
fdm-calculator/src/balance/nitrogen/performance.test.ts (1)
fdm-calculator/src/balance/nitrogen/types.d.ts (2)
FertilizerDetail(434-444)CultivationDetail(420-429)
fdm-calculator/src/balance/nitrogen/input.test.ts (1)
fdm-calculator/src/balance/nitrogen/types.d.ts (1)
FertilizerDetail(434-444)
fdm-calculator/src/balance/nitrogen/index.ts (5)
fdm-calculator/src/balance/nitrogen/supply/deposition.ts (1)
calculateAllFieldsNitrogenSupplyByDeposition(70-156)fdm-core/src/db/schema.ts (3)
fields(87-103)soilAnalysis(605-648)cultivations(289-301)fdm-calculator/src/balance/nitrogen/types.d.ts (2)
NitrogenBalanceField(323-348)FieldInput(395-415)fdm-calculator/src/balance/nitrogen/supply/index.ts (1)
calculateNitrogenSupply(29-79)fdm-calculator/src/conversions/soil.ts (3)
calculateOrganicCarbon(13-30)calculateOrganicMatter(40-57)calculateCarbonNitrogenRatio(69-89)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (4)
fdm-core/src/db/schema.ts (2)
cultivations(289-301)fertilizerApplication(180-197)fdm-calculator/src/balance/nitrogen/types.d.ts (4)
FieldInput(395-415)CultivationDetail(420-429)FertilizerDetail(434-444)NitrogenEmissionAmmoniaFertilizers(190-251)fdm-calculator/src/balance/nitrogen/emission/ammonia/index.ts (1)
calculateNitrogenEmissionViaAmmonia(21-51)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/index.ts (1)
calculateNitrogenEmissionViaAmmoniaByFertilizers(21-75)
🪛 GitHub Actions: Tests
fdm-data/src/fertilizers/hash.test.ts
[error] 65-65: AssertionError: expected '08585267' to be '5a405bbe' // Object.is equality
🪛 LanguageTool
.changeset/major-donkeys-rest.md
[grammar] ~5-~5: There might be a mistake here.
Context: ... of p_ef_nh3 to be a fraction instead of percentage
(QB_NEW_EN)
🪛 Biome (2.1.2)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts
[error] 242-289: This else clause can be omitted because previous branches break early.
Safe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 265-289: This else clause can be omitted because previous branches break early.
(lint/style/noUselessElse)
🔇 Additional comments (10)
fdm-data/src/fertilizers/d.ts (1)
22-22: Add JSDoc to clarify units forp_ef_nh3No occurrences of dividing or multiplying by 100 on
p_ef_nh3remain—all consumers already treat it as a 0–1 fraction. Please apply:export interface CatalogueFertilizerItem { @@ - p_ef_nh3?: number | null + /** + * Ammonia emission factor (fraction 0–1). Formerly expressed as percentage. + */ + p_ef_nh3?: number | nullfdm-data/src/fertilizers/hash.test.ts (1)
13-13: Good: ensure new field participates in hashing.Including
p_ef_nh3in the test object is correct so the hash reflects the new data shape.fdm-data/src/fertilizers/catalogues/baat.json (1)
7-7: p_ef_nh3 now in fraction units (0..1): alignment looks correctThese examples confirm the catalogue values have been scaled from percent to fraction, which matches the new ammonia calculator’s usage (no percent-to-fraction division in code).
Also applies to: 47-47, 167-167, 1087-1088, 1127-1128, 1287-1288, 1327-1328, 1407-1409, 1527-1529, 1567-1569, 1607-1609, 1647-1649, 1687-1689, 2247-2249, 2287-2289, 2327-2329, 2727-2729, 2767-2769
fdm-calculator/src/balance/nitrogen/performance.test.ts (1)
66-75: Confirm b_centroid shape across the codebase (tuple vs GeoJSON Point)This file sets b_centroid as [lon, lat]. Other tests use GeoJSON. Deposition code destructures as a tuple. Please standardize.
If needed, I can propose a compatibility helper to accept both tuple and GeoJSON Point in the deposition module.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (1)
203-222: Land-type detection depends on b_lu_croprotation values being "crop"/"grass"Current logic only recognizes "crop" or "grass". Callers passing “maize/wheat/etc.” will be misclassified as “bare soil.”
Would you like a defensive fallback that treats any non-grass value as “cropland”, or a mapping layer for known crop rotations?
fdm-calculator/src/balance/nitrogen/input.test.ts (1)
82-100: Standardize b_centroid shape to match downstream usageOther modules (deposition) destructure b_centroid as [lon, lat]. Here it’s a GeoJSON Point. Please align to avoid runtime shape mismatches across tests.
If you want, I can provide a small helper in deposition to accept both tuple and GeoJSON Point.
fdm-calculator/src/balance/nitrogen/index.ts (4)
56-63: Batched deposition prefetch: good move for performancePrefetching and reusing the GeoTIFF avoids per-field overhead and directly addresses the timeout issue for large farms.
155-170: Field-local timeframe normalization prevents inverted intervalsIntersecting with the global timeframe and clamping end>=start avoids downstream surprises. Nice defensive addition.
241-271: Correct: aggregate ammonia only at farm level per learningsUsing emission.ammonia.total in weighting aligns with the intended balance definition.
388-410: Good fallback conversions for missing soil parametersThe estimation chain for a_c_of, a_som_loi, a_cn_fr, and a_density_sa improves robustness of field runs with sparse data.
…l/undefined check and clamp)
| b_sampling_date: new Date("2023-05-01"), | ||
| a_som_loi: 5, | ||
| b_gwl_class: "HIGH", | ||
| }, |
There was a problem hiding this comment.
is this an existing option for b_gewl_class? Option "HIGH"?
gerardhros
left a comment
gerardhros
left a comment
There was a problem hiding this comment.
LGTM. unit checks are still running on github. you marked one issue of coderabbit asl solved for that the fertilizer date that is not null anymore. no explanation is given below, but i assume its correct.
Technically the |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (2)
35-47: Guard against missing application date to avoid runtime throws and data-dependent crashesp_app_date may be null in production data (DB schema does not enforce NOT NULL). Downstream code calls getTime() and will throw. Short-circuit here to keep the flow resilient.
Apply this diff:
const aggregatedEmissions = fertilizerApplications.reduce( (acc, application) => { const fertilizerDetail = fertilizerDetailsMap.get( application.p_id_catalogue, ) if (!fertilizerDetail) { throw new Error( `Fertilizer application ${application.p_app_id} has no fertilizerDetails`, ) } + // Guard against missing application date; skip malformed records + if (!application.p_app_date) { + console.warn( + `Missing application date for application ${application.p_app_id}; skipping`, + ) + return acc + }
185-194: Guard inside manure factor function to prevent getTime() on null dateDouble-defensive guard: even if the reducer skips, keep this function safe on direct calls and future refactors.
Apply this diff:
function determineManureAmmoniaEmissionFactor( fertilizerApplication: FertilizerApplication, cultivations: FieldInput["cultivations"], cultivationDetails: Map<string, CultivationDetail>, -) { +): Decimal { const p_app_name = fertilizerApplication.p_name_nl const p_id = fertilizerApplication.p_id const p_app_date = fertilizerApplication.p_app_date const p_app_method = fertilizerApplication.p_app_method + + if (!p_app_date) { + console.warn( + `Missing application date for ${p_app_name} (${p_id}); returning 0 emission factor`, + ) + return new Decimal(0) + }
🧹 Nitpick comments (6)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (6)
85-96: Optional: clamp manure/compost/other emission factor defensivelydetermineManureAmmoniaEmissionFactor currently returns sane values, but a defensive clamp keeps the path robust if factors change.
Apply this diff:
emissionFactor = determineManureAmmoniaEmissionFactor( application, cultivations, cultivationDetailsMap, ) + // Defensive clamp in case factors evolve + if (emissionFactor.lt(0)) emissionFactor = new Decimal(0) + if (emissionFactor.gt(1)) emissionFactor = new Decimal(1)
137-151: Clarify return semantics in JSDoc (fraction vs percent, clamping responsibility)The caller clamps determineMineralAmmoniaEmissionFactor’s output to [0..1]. Document this to prevent unit/semantics drift.
Apply this diff:
- * @returns A Decimal representing the calculated ammonia emission factor. + * @returns A Decimal representing the raw (unclamped) ammonia emission factor + * as a fraction (ideal range 0..1). The caller clamps to [0, 1].
160-160: Typo: “inhbiitor” → “inhibitor”Minor spelling fix in TODO comment.
Apply this diff:
- const p_inhibitor = false // TODO: implement inhbiitor details for fertilizers + const p_inhibitor = false // TODO: implement inhibitor details for fertilizers
195-218: Hot path optimization: avoid O(#cultivations) scan per application by memoizing land type per dateOn large farms, scanning cultivations for every application creates O(A·C) work. Precompute a resolver once per field with memoization by epoch day (or ISO date) to reduce this to amortized O(1) per call.
Proposed shape outside the reducer:
function buildLandTypeResolver( cultivations: FieldInput["cultivations"], details: Map<string, CultivationDetail>, ) { const cache = new Map<number, "grassland" | "cropland" | "bare soil">() // Optional: pre-sort cultivations by start date for predictable traversal const sorted = [...cultivations].sort( (a, b) => a.b_lu_start.getTime() - b.b_lu_start.getTime(), ) return (date: Date) => { const key = new Date(date.getFullYear(), date.getMonth(), date.getDate()).getTime() const hit = cache.get(key) if (hit) return hit // Compute once, then memoize let hasGrass = false, hasCrop = false for (const c of sorted) { if (c.b_lu_start.getTime() <= date.getTime() && (!c.b_lu_end || c.b_lu_end.getTime() >= date.getTime())) { const t = details.get(c.b_lu_catalogue)?.b_lu_croprotation if (t === "crop") hasCrop = true else if (t === "grass") hasGrass = true } } const land = hasCrop ? "cropland" : hasGrass ? "grassland" : "bare soil" cache.set(key, land) return land } }Then call the resolver from determineManureAmmoniaEmissionFactor instead of filtering every time.
228-294: Simplify control flow to satisfy noUselessElse (Biome) and improve readabilityPrevious branches return; the else/else-if is unnecessary. Flatten the structure.
Apply this diff:
- // Determine the Emission factor based on active cultivation type - if (landType === "grassland") { + // Determine the emission factor based on active cultivation type + if (landType === "grassland") { if (p_app_method === "broadcasting") { return new Decimal(0.68) } if (p_app_method === "narrowband") { return new Decimal(0.264) } if (p_app_method === "slotted coulter") { return new Decimal(0.217) } if (p_app_method === "shallow injection") { return new Decimal(0.17) } console.warn( `Unsupported application method ${p_app_method} for ${p_app_name} (${p_id}) on grassland`, ) return new Decimal(0) - } else if (landType === "cropland") { + } + if (landType === "cropland") { if (p_app_method === "broadcasting") { return new Decimal(0.69) } if (p_app_method === "incorporation 2 tracks") { return new Decimal(0.46) } if (p_app_method === "narrowband") { return new Decimal(0.36) } if (p_app_method === "slotted coulter") { return new Decimal(0.3) } if (p_app_method === "shallow injection") { return new Decimal(0.24) } if (p_app_method === "incorporation") { return new Decimal(0.22) } console.warn( `Unsupported application method ${p_app_method} for ${p_app_name} (${p_id}) for cropland`, ) return new Decimal(0) - } else { - // Bare soil + } + // Bare soil if (p_app_method === "broadcasting") { return new Decimal(0.69) } if (p_app_method === "incorporation 2 tracks") { return new Decimal(0.46) } if (p_app_method === "narrowband") { return new Decimal(0.36) } if (p_app_method === "slotted coulter") { return new Decimal(0.3) } if (p_app_method === "shallow injection") { return new Decimal(0.25) } if (p_app_method === "incorporation") { return new Decimal(0.22) } console.warn( `Unsupported application method ${p_app_method} for ${p_app_name} (${p_id}) for bare soil`, ) return new Decimal(0) - }
228-246: Consider reducing noisy console.warn in hot pathsOn large datasets, per-application warnings can be very chatty. Consider gating with a debug flag or aggregating counts per method/land type at the end.
I can sketch a lightweight warnOnce helper to avoid duplicate messages if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: SvenVw
PR: SvenVw/fdm#0
File: :0-0
Timestamp: 2025-08-13T10:33:05.313Z
Learning: In the fdm project, fdm-calculator integration for new features like b_lu_variety is handled in separate updates from the core data model changes. When fdm-core functions are updated to support new fields, fdm-calculator can consume these enhanced APIs without requiring changes in the same PR that introduces the core functionality.
📚 Learning: 2025-08-13T10:33:05.313Z
Learnt from: SvenVw
PR: SvenVw/fdm#0
File: :0-0
Timestamp: 2025-08-13T10:33:05.313Z
Learning: In the fdm project, fdm-calculator integration for new features like b_lu_variety is handled in separate updates from the core data model changes. When fdm-core functions are updated to support new fields, fdm-calculator can consume these enhanced APIs without requiring changes in the same PR that introduces the core functionality.
Applied to files:
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts
🧬 Code Graph Analysis (1)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (2)
fdm-core/src/db/schema.ts (2)
cultivations(289-301)fertilizerApplication(180-197)fdm-calculator/src/balance/nitrogen/types.d.ts (4)
FieldInput(395-415)CultivationDetail(420-429)FertilizerDetail(434-444)NitrogenEmissionAmmoniaFertilizers(190-251)
🪛 Biome (2.1.2)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts
[error] 246-293: This else clause can be omitted because previous branches break early.
Safe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 269-293: This else clause can be omitted because previous branches break early.
(lint/style/noUselessElse)
🔇 Additional comments (5)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (5)
21-33: Solid consolidation and clear accumulator shapeGood centralization of the ammonia-by-fertilizers path with a single pass reducer and a well-structured result object. This makes future optimizations and caching easier.
41-45: Decide: throw vs skip on missing fertilizerDetailsThrowing here aborts the whole field’s emissions. If catalogue joins can be incomplete in real data, consider logging and skipping to avoid breaking the entire nitrogen balance on large farms.
Would you confirm whether fertilizerDetailsMap is guaranteed complete for all p_id_catalogue values by the time this runs? If not, I can provide a safe “warn-and-skip” alternative.
58-69: Good: 0 emission factor is honored and clamped to [0..1]Switching to a nullish check (!= null) preserves valid 0 values, and clamping prevents negative or >1 factors. Exactly the right fix.
56-64: FertilizerApplication fields confirmed – no action requiredThe
FertilizerApplicationinterface in@svenvw/fdm-core(seesrc/fertilizer.d.ts) exports both
•p_id_catalogue: string
•p_name_nl: string | nullNo additional joins or safe resolvers are needed for these properties.
129-134: No changes needed – negative sign convention confirmedUnit tests for calculateNitrogenEmissionViaAmmoniaByFertilizers all assert negative totals (e.g. expect(result.total.toFixed…) toBe("-60.29600")), so the summed
aggregatedEmissions.totalcorrectly produces a negative value by design.
2 participants
Summary by CodeRabbit
Performance
New Features
Breaking Changes
Tests
Closes #250