Add function to calculate nitrogen balance#134
Conversation
…ionsType` to `Fertilizer` and `FertilizerApplication`
…put data from a fdm instance to calculate the nitrogen balance
🦋 Changeset detectedLatest commit: 3bee620 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 |
WalkthroughThis update introduces a comprehensive nitrogen balance calculation feature for agricultural fields and farms. It adds new soil and cultivation properties, expands type definitions, updates database schemas, and implements detailed calculation modules, including supply, removal, and volatilization of nitrogen. Extensive test suites and utility functions support robust data processing and validation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Calculator
participant CoreDB
participant PublicData
User->>App: Request nitrogen balance for farm
App->>Calculator: collectInputForNitrogenBalance(farmId, timeframe)
Calculator->>CoreDB: getFields, getCultivations, getHarvests, getSoilAnalyses, getFertilizerApplications, getFertilizers, getCultivationsFromCatalogue
CoreDB-->>Calculator: Returns all required data
Calculator->>Calculator: Assemble NitrogenBalanceInput
App->>Calculator: calculateNitrogenBalance(NitrogenBalanceInput)
Calculator->>PublicData: Fetch deposition raster (GeoTIFF)
Calculator->>Calculator: Compute supply, removal, volatilization per field
Calculator->>Calculator: Aggregate to farm-level balance
Calculator-->>App: Return NitrogenBalanceNumeric
App-->>User: Display nitrogen balance results
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #134 +/- ##
===============================================
+ Coverage 92.99% 93.43% +0.44%
===============================================
Files 47 65 +18
Lines 6135 7327 +1192
Branches 698 899 +201
===============================================
+ Hits 5705 6846 +1141
- Misses 429 479 +50
- Partials 1 2 +1
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:
|
| total: residues.total, // Ammonia total should include residues total | ||
| fertilizers: { | ||
| total: Decimal(0), // Fertilizers volatilization not yet implemented | ||
| mineral: { |
There was a problem hiding this comment.
you might set it to the lowest value (EF or CAN) rather than zero.
There was a problem hiding this comment.
This would lead to provide a wrong number as answer, better to give no answer until the calculation rules are clear for all sitatuations
| }, | ||
| manure: { | ||
| total: Decimal(0), | ||
| applications: [], |
There was a problem hiding this comment.
same here, you might select the value for most commont slurry and application technology as default EF.
There was a problem hiding this comment.
This would lead to provide a wrong number as answer, better to give no answer until the calculation rules are clear for all sitatuations
| "multiple", | ||
| ]) | ||
| export const rotationEnum = fdmSchema.enum("b_lu_croprotation", [ | ||
| "other", |
There was a problem hiding this comment.
name croprotation might be confusing here, since these are more crop categories than a rotation class, isn't? Might be renamed later
There was a problem hiding this comment.
Please provide a better name, changing later is more complicated
gerardhros
left a comment
gerardhros
left a comment
There was a problem hiding this comment.
Well done Sven. See some minor comments
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
fdm-calculator/src/conversions/soil.ts (1)
69-87: Past review comments successfully addressed.The JSDoc documentation now correctly specifies that
a_n_rtis in mg N/kg soil, and the/1000conversion properly converts to g N/kg soil for the C/N ratio calculation. The implementation is correct and the boundary constraints (5-40) are appropriate for agricultural soils.fdm-core/src/cultivation.ts (1)
111-116: 🛠️ Refactor suggestion[DUPLICATE COMMENT] Consider adding validation for nitrogen-related parameters.
As noted in previous reviews, the new nitrogen-related parameters (
b_lu_croprotation,b_lu_yield,b_lu_hi,b_lu_n_harvestable,b_lu_n_residue,b_n_fixation) lack validation. Consider adding range checks to ensure data integrity before database insertion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/clear-guests-taste.md(1 hunks).changeset/fine-symbols-taste.md(1 hunks).changeset/swift-colts-watch.md(1 hunks).changeset/true-windows-scream.md(1 hunks)fdm-calculator/src/balance/nitrogen/index.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/input.ts(1 hunks)fdm-calculator/src/balance/nitrogen/volatization/residues.ts(1 hunks)fdm-calculator/src/conversions/soil.ts(1 hunks)fdm-core/src/cultivation.ts(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .changeset/true-windows-scream.md
- .changeset/fine-symbols-taste.md
- fdm-calculator/src/balance/nitrogen/volatization/residues.ts
- .changeset/swift-colts-watch.md
- fdm-calculator/src/balance/nitrogen/input.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
fdm-calculator/src/conversions/soil.ts (1)
fdm-core/src/db/schema.ts (1)
soilAnalysisTypeSelect(531-531)
🪛 LanguageTool
.changeset/clear-guests-taste.md
[uncategorized] ~5-~5: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...s property of cultivation catalogue. It represent the amount of nitrogen that can be fixa...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🪛 GitHub Check: codecov/patch
fdm-calculator/src/conversions/soil.ts
[warning] 122-122: fdm-calculator/src/conversions/soil.ts#L122
Added line #L122 was not covered by tests
[warning] 125-125: fdm-calculator/src/conversions/soil.ts#L125
Added line #L125 was not covered by tests
fdm-core/src/cultivation.ts
[warning] 758-771: fdm-core/src/cultivation.ts#L758-L771
Added lines #L758 - L771 were not covered by tests
🔇 Additional comments (8)
fdm-calculator/src/conversions/soil.ts (3)
13-30: LGTM! Well-implemented organic carbon calculation.The function correctly implements the standard soil science conversion from organic matter to organic carbon content. The use of Decimal.js for precision, proper null handling, and realistic boundary constraints (0.1-600 g C/kg soil) demonstrate good implementation practices.
40-57: LGTM! Proper inverse calculation implementation.The function correctly implements the mathematical inverse of
calculateOrganicCarbon. The boundary constraints (0.5-75%) are appropriate for soil organic matter percentages, and the implementation follows the same quality patterns as the previous function.
100-129: Address test coverage gaps for boundary conditions.The bulk density calculation logic is well-implemented with appropriate formulas for different soil types. However, the upper and lower boundary clamping conditions (lines 122 and 125) lack test coverage.
Please add test cases to cover the boundary conditions:
// Test case for upper boundary (> 3 kg/m³) it('should clamp bulk density to maximum of 3 kg/m³', () => { // Test with very low organic matter that would produce high density const result = calculateBulkDensity(0.1, 'klei'); expect(result).toBe(3); }); // Test case for lower boundary (< 0.5 kg/m³) it('should clamp bulk density to minimum of 0.5 kg/m³', () => { // Test with very high organic matter that would produce low density const result = calculateBulkDensity(75, 'dekzand'); expect(result).toBe(0.5); });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 122-122: fdm-calculator/src/conversions/soil.ts#L122
Added line #L122 was not covered by tests
[warning] 125-125: fdm-calculator/src/conversions/soil.ts#L125
Added line #L125 was not covered by testsfdm-calculator/src/balance/nitrogen/index.test.ts (1)
1-197: LGTM! Well-structured test suite with comprehensive coverage.The test file demonstrates excellent practices:
- Comprehensive mock data including all nitrogen-related properties
- Proper error handling validation
- Realistic test values (addressing previous feedback)
- Good separation of concerns with focused test cases
- Proper type checking in assertions
The tests effectively validate the nitrogen balance calculation functionality and integration with the updated data model.
fdm-core/src/cultivation.ts (4)
158-159: LGTM! Consistent integration of crop residue flag.The
m_cropresidueproperty has been properly integrated across all cultivation functions:
- Parameter documentation updated
- Database operations include the field
- Function signatures consistently updated
- Return types properly reflect the new property
This provides valuable information for nitrogen balance calculations regarding crop residue management.
Also applies to: 170-170, 279-279, 354-354, 441-441, 495-495, 587-587, 720-720, 872-872, 886-886, 1009-1009
758-771: LGTM! Harvest structure successfully updated to single object.The change from
harvestablesarray to singleharvestableobject has been implemented correctly:
- Proper object structure with nested
harvestable_analysesarray- All required fields included in the mapping
- Field selection appears complete (addressing previous review concerns)
This aligns with the broader data model updates mentioned in the AI summary.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 758-771: fdm-core/src/cultivation.ts#L758-L771
Added lines #L758 - L771 were not covered by tests
15-19: LGTM! Improved type safety with proper type imports.The replacement of generic return types with specific interfaces (
Cultivation,CultivationPlan,CultivationCatalogue) enhances type safety and code clarity. This addresses previous concerns about type annotations.Also applies to: 44-44, 331-331, 409-409, 554-554
597-597:Details
✅ Verification successful
Verify that
b_id_harvestablefield selection resolves previous runtime issue.The
b_id_harvestablefield is now included in the select clause, which should resolve the previous issue where this field was undefined at runtime. This field is properly used in the harvest mapping on line 759.
🏁 Script executed:
#!/bin/bash # Verify that b_id_harvestable is properly selected and used ast-grep --pattern $'b_id_harvestable: schema.harvestables.b_id_harvestable'Length of output: 77
I couldn’t locate any instances of that snippet using
ast-grep. Let’s search the file directly and inspect the nearby lines to confirm thatb_id_harvestableis indeed selected and used:
🏁 Script executed:
#!/bin/bash # Search for any occurrences of b_id_harvestable rg -n "b_id_harvestable" fdm-core/src/cultivation.ts # Show lines 580–620 to inspect the select clause around line 597 sed -n '580,620p' fdm-core/src/cultivation.tsLength of output: 2978
Confirmed:
b_id_harvestableis properly selected and usedI’ve verified that in fdm-core/src/cultivation.ts:
- Line 597: the SELECT clause includes
b_id_harvestable: schema.harvestables.b_id_harvestable- Line 759: the harvest mapping uses
b_id_harvestable: curr.b_id_harvestableThis ensures the field is no longer undefined at runtime.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <37927107+SvenVw@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.ts (1)
11-11: Fix typo in documentation.The documentation contains a typo: "nnot of any type type" should be corrected.
- * This function iterates through the provided fertilizer applications, identifies those that are nnot of any type type, + * This function iterates through the provided fertilizer applications, identifies those that are not mineral, manure, or compost,fdm-calculator/src/balance/nitrogen/supply/fertilizers/index.ts (2)
43-43: Fix typo in comment.- // Calculate the amount of Nitrogen supplied by othyer fertilizers + // Calculate the amount of Nitrogen supplied by other fertilizers
60-60: Add missing semicolon.- other: fertilizersSupplyOther + other: fertilizersSupplyOther,fdm-calculator/src/balance/nitrogen/types.d.ts (3)
73-73: Fix typo in documentation.- * A detailed list of individual other fertilzer applications. + * A detailed list of individual other fertilizer applications.
131-131: Use consistent spelling for mineralization.The codebase uses both "mineralisation" and "mineralization". For consistency, standardize on one spelling (preferably "mineralization" as used elsewhere).
- mineralisation: NitrogenSupplyMineralization + mineralization: NitrogenSupplyMineralization- mineralisation: NitrogenSupplyMineralizationNumeric + mineralization: NitrogenSupplyMineralizationNumericAlso applies to: 435-435
423-424: Remove duplicate comment.-// Numeric version of NitrogenSupplyMineralization // Numeric version of NitrogenSupplyMineralization
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
fdm-calculator/src/balance/nitrogen/supply/fertilizers/index.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/index.ts(1 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.ts(1 hunks)fdm-calculator/src/balance/nitrogen/types.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/index.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.test.ts (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (2)
FieldInput(340-359)FertilizerDetail(378-385)fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.ts (1)
calculateNitrogenSupplyByOtherFertilizers(18-69)
fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.ts (1)
fdm-calculator/src/balance/nitrogen/types.d.ts (3)
FieldInput(340-359)FertilizerDetail(378-385)NitrogenSupplyFertilizers(17-78)
fdm-calculator/src/balance/nitrogen/types.d.ts (1)
fdm-calculator/src/index.ts (2)
NitrogenBalanceInput(7-7)NitrogenBalanceNumeric(8-8)
🔇 Additional comments (1)
fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.test.ts (1)
1-87: Well-structured and comprehensive test suite!The test cases effectively cover all scenarios: empty input, normal calculations, and error handling. The mathematical calculations are correct and the use of Decimal.js ensures precision.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <37927107+SvenVw@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <37927107+SvenVw@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <37927107+SvenVw@users.noreply.github.com>
…into add-calculate-nitrogen-balance
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores