Skip to content

Prioritize kroend over krowend/krogend - #504

Open
bartekvik wants to merge 1 commit into
equinor:mainfrom
bartekvik:Kroend_prioritize
Open

Prioritize kroend over krowend/krogend#504
bartekvik wants to merge 1 commit into
equinor:mainfrom
bartekvik:Kroend_prioritize

Conversation

@bartekvik

@bartekvik bartekvik commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

This PR modifies the functionality introduced in #471 to prioritize kroend (legacy) over krowend/krogend. This is in relation to a condensate system.

Background
PR #471 introduced support for phase-specific endpoint keys ( krowend for WaterOil and krogend for GasOil).
Some existing workflows still provide kroend, and rely on it being used when multiple endpoint keys are present.

PR Changes

  • Keep support for phase-specific keys krowend, krogend
  • Prioritizes kroend when it is present together with the phase-specific keys
    • If kroend contains a valid value, it is used (highest priority).
    • If kroend is missing or contains NaN, kroend is treated as unavailable and the implementation falls back to the phase-specific endpoints:
      • krowend maps to WaterOil kroend.
      • krogend maps to GasOil kroend.

This ensures correct behavior for the edge case where kroend is present in the input but its value is NaN.

  • Emits warnings when multiple endpoint keys are provided with non-NaN values to encourage migration toward the phase-specific keys (krowend/krogend).
  • Updates/adds tests to verify:
    • priority order of input keys
    • warning behavior
    • normalization behavior
    • NaN-aware endpoint handling:
      -fallback to phase-specific keys when kroend is NaN
      -keep kroend when phase-specific keys are NaN
      -no warning when one side is NaN
      -phase-specific keys are removed from output representations

Behavior

  • If kroend is present: it is used (highest priority).
  • If kroend is absent:
    • krowend maps to WaterOil kroend.
    • krogend maps to GasOil kroend.
    • NaN is treated as missing for endpoint selection.
    • Irrelevant phase key is removed per phase during normalization.
  • Warning is only logged when both legacy and canonical values are present and non-NaN.

Backward Compatibility

  • Existing inputs using kroend continue to work as expected.
  • Existing mixed-input cases now consistently favor legacy kroend.
  • Users are informed via warnings when legacy input is detected.

@codecov-commenter

codecov-commenter commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.80%. Comparing base (b1a0afa) to head (95efa64).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #504   +/-   ##
=======================================
  Coverage   98.79%   98.80%           
=======================================
  Files          18       18           
  Lines        2664     2682   +18     
=======================================
+ Hits         2632     2650   +18     
  Misses         32       32           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Treat NaN kroend as missing and fall back to legacy endpoints

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates three-phase factory endpoint normalization to prioritize legacy kroend while retaining phase-specific fallback behavior.

Changes:

  • Adds NaN-aware endpoint selection and warning behavior.
  • Expands tests for priority, normalization, and NaN handling.
  • Identified outdated public documentation requiring correction.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/pyscal/factory.py Implements endpoint priority and fallback logic.
tests/test_factory.py Tests priority, warnings, normalization, and NaN cases.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/pyscal/factory.py
Comment on lines +1427 to +1430
- If only 'krowend' is present and not NaN, map it to 'kroend'.
- If both 'krowend' and 'kroend' are present, treat NaN as missing for
endpoint selection; when both are valid, legacy 'kroend' is prioritized.
- Log a warning only when both are present and both are not NaN.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants