Feature/v3/feature/305 rename specific share to other effects to specific share from effect

[FBumann]

Description

Rename specific_effects_from_ ... WITHOUT DEPRECTATION

Type of Change

• Bug fix
• New feature
• Documentation update
• Code refactoring

Related Issues

Closes #(issue number)

Testing

• I have tested my changes
• Existing tests still pass

Checklist

• My code follows the project style
• I have updated documentation if needed
• I have added tests for new functionality (if applicable)

Summary by CodeRabbit

• New Features
  • Introduced clearer effect sharing parameters: share_from_temporal and share_from_nontemporal for cross-effect and non-temporal contributions.
  • Added configurable delimiter for effect label composition.
• Removed (Breaking Changes)
  • Removed specific_share_to_other_effects_operation and specific_share_to_other_effects_invest (mapping provided; direction inverted). No deprecation period.
• Documentation
  • Unreleased changelog updated to reflect the new API and removals.
  • Examples updated to use the new sharing syntax (e.g., Costs receiving a 20% share from CO2).

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Renamed cross-effect sharing parameters to share_from_temporal/share_from_nontemporal and inverted share direction (targets receive from sources). Updated Effect API, internal modeling/collection logic, and data fitting. Adjusted examples and tests to new syntax. Added a delimiter argument to fit_effects_to_model_coords. Updated changelog.

Changes

Cohort / File(s)	Summary
Core Effect API and sharing logic flixopt/effects.py	Replaced specific_share_to_other_effects_operation/invest with share_from_temporal/share_from_nontemporal across constructor, attributes, transform_data, share factor calculations, and modeling/collection propagation; implemented target_effect <- source_effect mapping.
FlowSystem fitting flixopt/flow_system.py	Added delimiter parameter to fit_effects_to_model_coords; replaced hardcoded '
Examples updated to new API examples/01_Simple/simple_example.py, examples/02_Complex/complex_example.py, examples/04_Scenarios/scenario_example.py	Costs effects now declare share_from_temporal={'CO2': 0.2}; removed specific_share_to_other_effects_operation from CO2 effects.
Tests and fixtures migrated tests/conftest.py, tests/test_effect.py, tests/test_scenarios.py	Introduced costs_with_co2_share fixture; removed co2_with_costs_share; updated Effect instantiations to share_from_temporal/share_from_nontemporal and expectations to new share direction.
Changelog CHANGELOG.md	Documented new sharing syntax and removal of old parameters, including direction inversion mapping.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant User as User code
  participant EC as EffectCollection
  participant E_costs as Effect: costs
  participant E_co2 as Effect: CO2
  participant FSM as FlowSystemModel

  rect #eef7ff
    note right of User: Define effects with new API
    User->>E_costs: Effect(..., share_from_temporal={'CO2': 0.2})
    User->>E_co2: Effect('CO2', ...)
  end

  rect #f6ffed
    note over EC,E_costs: Build models and align data
    User->>EC: add(E_costs, E_co2)
    EC->>FSM: fit_effects_to_model_coords(..., delimiter='|')
    EC->>EC: calculate_effect_share_factors()<br/>(targets read sources)
  end

  rect #fff7e6
    note over EC,E_costs: Apply temporal shares
    EC->>E_costs: pull 0.2 share_from_temporal['CO2']
    E_co2-->>E_costs: provide temporal contribution
  end

  note over E_costs: costs receives share from CO2 (direction inverted)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Feature/308 rename effect domains #360 — Also changes Effect API around temporal/nontemporal naming; overlaps with effects.py refactors.
• Feature/v3/feature/308 rename effect domains #365 — Adjusts cross-effect share modeling in effects.py; closely aligned with this share direction/name change.
• Feature/v3/update #352 — Modifies Effect.transform_data and FlowSystem.fit_effects_to_model_coords signatures/behavior, touching the same code paths.

Suggested labels

v3.0.0

Suggested reviewers

• baumbude
• PStange

Poem

I tap my paw on temporal ground,
Costs now listen where CO2 is found.
Shares hop upstream, direction anew—
From source to target, a cleaner view.
With pipes or custom seams we thread,
A tidy warren for models to tread. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Title Check	⚠️ Warning	The pull request title is overly verbose and includes branch metadata rather than a concise summary of the main change; it does not clearly state that specific_share_to_other_effects parameters are being renamed to share_from_temporal and share_from_nontemporal in the Effect API.	Please rewrite the title to a short, clear sentence that highlights the key change, for example "Rename specific_share_to_other_effects parameters to share_from_temporal and share_from_nontemporal in Effect API".
Description Check	⚠️ Warning	While the description follows the template structure, the Description section is too terse and uses ellipses, the Related Issues section has a placeholder instead of a real issue number, and the Testing section is left unchecked, so the PR description does not provide sufficient detail or verification status for reviewers.	Please expand the Description to clearly explain which parameters are being renamed and why, fill in the Related Issues field with the actual issue number, and update the Testing section by confirming that tests have been executed and still pass.
Docstring Coverage	⚠️ Warning	Docstring coverage is 29.41% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[FBumann]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (5)

CHANGELOG.md (1)

60-60: Fix typo in the share syntax description.

"rather then" should be "rather than" to keep the docs polished.

-This replaces the less intuitive `specific_share_to_other_effects_*` parameters and makes it clearer where effect contributions are coming from, rather then where they are going to.
+This replaces the less intuitive `specific_share_to_other_effects_*` parameters and makes it clearer where effect contributions are coming from, rather than where they are going to.

tests/conftest.py (1)

103-105: Convenience constructor is fine; consider avoiding duplication

Effects.costs_with_co2_share() mirrors the inline assignment used elsewhere. To reduce surface area drift, prefer one pattern (either helper or inline assignment) across fixtures.

flixopt/effects.py (3)

90-92: Doc example: clarify labels to prevent confusion

Example shows an effect labeled 'co2_emissions' receiving from 'CO2'. If the intended source label is the CO2 effect, use its exact label in the mapping to avoid runtime KeyErrors, or rename the example for consistency.

Apply this tweak for clarity:

-            label='co2_emissions',
+            label='CO2',
...
-            share_from_temporal={'CO2': 0.2},  # €0.2 per kg CO2
+            share_from_temporal={'CO2': 0.2},  # receives €0.2 per kg CO2 into costs if used on a cost effect

---

113-115: Doc example likely self-referential by mistake

primary_energy receiving from 'primary_energy' reads as a self-share. If the intention is to price primary energy into costs, consider updating the example to apply the share on a cost effect.

Proposed correction:

-        primary_energy = Effect(
-            label='primary_energy',
-            unit='kWh_primary',
-            description='Primary energy consumption',
-            share_from_temporal={'primary_energy': 0.08},  # €0.08/kWh
-        )
+        costs = Effect(
+            label='system_costs',
+            unit='€',
+            description='Total system costs',
+            share_from_temporal={'primary_energy': 0.08},  # €0.08 per kWh_primary
+        )

---

735-749: Wiring of cross-effect shares is correct; add early validation for unknown sources

The submodel-level adds are correct. Consider an early, friendlier check that all source_effect keys exist before modeling to fail fast with actionable messages.

For example, augment _plausibility_checks:

 def _plausibility_checks(self) -> None:
     # Check circular loops in effects:
     temporal, nontemporal = self.calculate_effect_share_factors()
+    # Validate all referenced sources exist
+    unknown = {
+        src for src, _ in list(temporal.keys()) + list(nontemporal.keys())
+        if src not in self.effects
+    }
+    if unknown:
+        raise KeyError(f"Unknown effects referenced in share mappings: {sorted(unknown)}")

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8ee16e4 and 1d77989.

📒 Files selected for processing (9)

• CHANGELOG.md (2 hunks)
• examples/01_Simple/simple_example.py (1 hunks)
• examples/02_Complex/complex_example.py (1 hunks)
• examples/04_Scenarios/scenario_example.py (1 hunks)
• flixopt/effects.py (9 hunks)
• flixopt/flow_system.py (2 hunks)
• tests/conftest.py (4 hunks)
• tests/test_effect.py (2 hunks)
• tests/test_scenarios.py (1 hunks)

🔇 Additional comments (13)

tests/test_scenarios.py (1)

127-128: Adopts new share_from_temporal API correctly

Costs now receives 20% from CO2 temporals; ordering is fine since submodels are created before cross-effect wiring. Looks good.

tests/conftest.py (4)

107-109: CO2 factory retained

Static Effects.co2() keeps the public shape stable. Good.

---

386-387: Fixture migration to new API

Using costs_with_co2_share() + co2() aligns with the inverted share direction. Good.

---

416-417: Consistent usage across scenario fixture

Same as above; consistent and clear.

---

469-469: Inline assignment of share_from_temporal is valid
Verified no legacy specific_share_to_other_effects_* references remain in the codebase. Keep label spelling in sync with the actual Effect label to avoid KeyErrors at modeling time.

tests/test_effect.py (3)

159-171: Tests reflect inverted share direction

Effect2/Effect3 receiving from Effect1 via share_from_{temporal,nontemporal} matches the new semantics and expected variable/constraint names. Good.

---

228-242: Multi-hop temporal sharing covered

Chained shares (costs → Effect1 → Effect2/Effect3) are asserted via aggregated factors. This validates BFS path accumulation. Nice.

---

244-247: System composition updated correctly

Adding the three effects before components ensures share factors are available during modeling. Good.

flixopt/effects.py (5)

53-57: Public API docs updated

Parameters share_from_temporal / share_from_nontemporal are documented; clear that targets receive from sources. Good.

---

153-174: Constructor accepts new share mappings; defaults safe

Initialization to {} avoids None checks downstream. Type hints are fine under from __future__ import annotations. Good.

---

395-406: Transform aligns shares to model coords

Using fit_effects_to_model_coords(..., dims=['time','year','scenario']) for temporal and ['year','scenario'] for nontemporal is correct and future‑proofs broadcasting. Good.

---

417-419: Consistency for nontemporal bounds

Bounds are fit to ['year','scenario'], matching nontemporal submodel dims. Good.

---

663-677: Effect share factor aggregation inverted correctly

Mapping now builds source→target adjacency from target-side share_from_*. This matches the new API and supports transitive paths. Good.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Correct spelling of “deprecation”.

The emphasis is fine, but “DEPRECTATION” is misspelled. Please adjust to “deprecation” for clarity.

-* **Effect share parameters**: The old `specific_share_to_other_effects_*` parameters were replaced WITHOUT DEPRECTATION
+* **Effect share parameters**: The old `specific_share_to_other_effects_*` parameters were replaced without deprecation

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	* **Effect share parameters**: The old `specific_share_to_other_effects_*` parameters were replaced WITHOUT DEPRECTATION
	- `specific_share_to_other_effects_operation` → `share_from_temporal` (with inverted direction)
	* **Effect share parameters**: The old `specific_share_to_other_effects_*` parameters were replaced without deprecation
	- `specific_share_to_other_effects_operation` → `share_from_temporal` (with inverted direction)

🤖 Prompt for AI Agents

In CHANGELOG.md around lines 132 to 133, the word “DEPRECTATION” is misspelled;
replace it with the correct word “deprecation” so the sentence reads that the
old parameters were replaced WITHOUT deprecation (preserve capitalization for
emphasis).