Repository navigation
Conversation
Adds ravens-json to the transmission hub: the balanced subset of the LANL CIM-derived interchange schema (lanl-ansi/MG-RAVENS, profile mgravens24v1 on CIM100). Addresses #27 (RAVENS JSON export and ingest backend). The writer follows the upstream MATPOWER converter's conventions (mpc2ravens.py) so output drops into the RAVENS toolchain unchanged: SI units, ConnectivityNode per bus with inline SvVoltage, the slack marked by an OperationalLimitType named slack, machines in the CIM load sign convention, tap/shift branches as two-end PowerTransformer records with Ratio/PhaseTapChanger, ProducerCostFunction polynomial costs, and the MVA base in an AlgorithmSettings record. Deviations from upstream are deliberate and warned: deterministic name-derived mRIDs (canonical writes are idempotent), base-voltage anchors on lines and transformer ends (transit buses read back at the right base), and a 1 kV effective base when a case carries base_kv 0 so per-unit values still round-trip. The reader inverts that mapping, keeps mRIDs in element uids, folds LoadResponseCharacteristic into the ZIP/exponential voltage models, and refuses documents with multiconductor objects (per-phase records, impedance matrices, wire geometry) with a pointer at the distribution surface instead of dropping phase detail. Unknown top-level CIM object kinds warn with counts; everything unrepresentable on the write side is itemized in Conversion::warnings per the fidelity contract. Wiring: TargetFormat/SourceFormat::RavensJson, routing aliases (ravens, mg-ravens), bare-JSON auto-detection on the top-level ConnectivityNode / PowerSystemResource tables, CLI FormatArg, docs (format-fidelity chapter, README table, AGENTS.md), and a vendored Apache-2.0 upstream example under tests/data/ravens with provenance. serde_json gains float_roundtrip so parse(print(x)) == x holds for the canonical-write idempotence tests. Related: #237 (CIM/CGMES umbrella). (cherry picked from commit b866692; the reader moved from powerio/src/format/ravens.rs to powerio-tx/src/format/ravens.rs and its hub registration into the powerio-tx format hub, routing, and SourceFormat, where those files live now)
MG-RAVENS (lanl-ansi/MG-RAVENS, Apache-2.0) is the LANL/Triad microgrid data schema: one JSON document of name-keyed hash tables nested under the CIM class hierarchy, SI units, and Class::'name' pointers. This adds the distribution profile — the OpenDSS-derived multiconductor sibling of the balanced transmission profile in the powerio crate — into the wire-coordinate DistNetwork, so a RAVENS feeder converts to OpenDSS, PMD, and BMOPF and every distribution source converts to RAVENS. Reader (powerio-dist/src/ravens/read.rs): walks the class-hierarchy nesting into a flat index and maps ACLineSegment + ACLineSegmentPhase + PerLengthPhaseImpedance ohm/m matrices, EnergyConsumer + EnergyConsumerPhase (wye/delta/single-phase, ZIP/exponential LoadResponseCharacteristic), LinearShuntCompensator banks (including neutral-only grounding shunts), the Switch family with SwitchPhase side maps, SynchronousMachine + GeneratingUnit, PowerElectronicsConnection PV/battery units, EnergySource Thevenin sources, and TransformerTank ends with catalog TransformerEndInfo short-circuit / no-load tests and RatioTapChanger taps. Objects with no typed mapping are preserved untyped with their source path so they re-nest on write. Balanced transmission-profile documents are refused with a pointer to the powerio crate's reader. Writer (powerio-dist/src/ravens/write.rs): regenerates the document shape with deterministic UUID-shaped mRIDs (imported ids pass through), the fixed operational-limit vocabulary, and per-component bus base voltages. Every scaled emission finishes on a read/write fixed point, so write -> parse -> write is byte identical across the whole distribution fixture corpus (IEEE 13/34/123, the transformer micro-cases, and the BMOPF ENWL and IEEE 13 examples). Conductor counts above the four CIM SinglePhaseKind identities (A/B/C/N) are written in full but warned as unrecoverable. Wiring: DistTargetFormat::RavensJson and DistSourceFormat::RavensJson; dist_target_from_name and the .json auto-classifier recognize the RAVENS markers; the shared powerio routing classifier gains DistributionFormat::RavensJson so a bare .json auto-detects as distribution; CLI FormatArg::RavensJson (--to ravens-json, convert feeder.json --to dss). Fixture vendored from MG-RAVENS (case3_balanced.json, Apache-2.0). Tests: exact mapping, projection round trips, dss/pmd/bmopf conversions, full-corpus byte idempotence, and the routing classifier. Addresses #27 (MG-RAVENS support). Tracked in #237. (cherry picked from commit 8b7d62a0d; the DistTargetFormat, DistSourceFormat, and convert dispatch registration was re-applied onto the current module based powerio-dist, and the DistributionFormat routing entry moved to powerio-tx/src/format/routing.rs)
An adversarial review against the upstream converter (lanl-ansi/MG-RAVENS ravens/xml/opendss2xml.py) surfaced three conventions that round-tripped with themselves but disagreed with real MG-RAVENS documents: - SynchronousMachine active/reactive power is injection-negative in the CIM load convention (opendss2xml writes `-gen.kW`), so a positive generation setpoint must emit as negative RotatingMachine.p/q and read back positive. maxQ/minQ and GeneratingUnit bounds stay unnegated. - PowerElectronicsConnection (PV/battery) p and q take the same injection-negative convention (opendss2xml PV writes the OpenDSS load-convention solved power). - ShuntCompensator.nomU is cap.kV*1000 — line-to-line for a multi-phase bank, not the per-phase voltage — so the dss kv is nomU/1000 with no sqrt(3), and bPerSection = 0.001*kvar/kV^2 already equals the per-conductor susceptance (the sqrt(3) and 1/phases cancel), so the total reactive power is bPerSection*nomU^2 with no per-phase factor. Both the reader and writer are corrected symmetrically, so byte idempotence across the whole distribution fixture corpus is preserved while the emitted values now match upstream. New tests pin the machine/PV injection-negative sign and the shunt line-to-line nomU. The RatioTapChanger step heuristic (step in [0.5,1.5] read as a direct ratio) is confirmed correct against opendss2xml, which emits the tap ratio, not an integer position. Related: #27. Tracked in #237.
…MATICAL JSON MG-RAVENS JSON (`ravens-json`) parses and writes in both profiles under one token (#27). The balanced profile is a powerio-tx format, the multiconductor profile a powerio-dist format, `powerio_tnd::formats::ravens` picks the reader from the document's classes, and `powerio_core::cim` holds the vocabulary the CIM derived formats share: deterministic mRIDs, the phase letters, PhaseCode expansion, and profile detection. Mohamed Numair's readers and writers are ported onto the shared hubs with his commits preserved. The multiconductor writer states grounded terminals as CIM `Ground` objects, a wye load's return on the neutral, a four wire segment's neutral conductor, each segment end's own phase code, `Yn` for a bank with a grounded neutral, and an unbalanced source as one single phase EnergySource per conductor; a segment with a conductor outside the CIM phase identities is dropped with a finding. PowerModelsDistribution's MATHEMATICAL model parses and writes as `pmd-math-json` (#645): the writer follows `transform_data_model` (loss model transformers, Thevenin sources, calc_voltage_bases, make_per_unit!, Kron reduction, the `map`), iterating every value to the fixed point of read then rewrite so write, read, write is byte identical; the reader folds through the map, restores the neutral a Kron reduced wye load returns on, and restates winding impedances on the emergency rating. The tests compare the writer field by field against PMD v0.16.0 exports of four PMD cases. The round trips also fix the PMD ENGINEERING ZIP spelling (`ZIP` with `zipv`), a switch's linecode fields, an empty network name, the OpenDSS writer's `New Circuit.` for an empty name, and the JSON classifier's precedence of PMD's `data_model` marker over PowerModels shaped tables. The conversion matrix gains both formats and two cases, loads an OpenDSS readback's bus coordinate sidecar, and names the codes of error severity findings; its baseline is regenerated. Co-authored-by: Robert Ferrando <68822690+ferrandor@users.noreply.github.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017y9wBoq8zZDx3ypp7qQF6q
Conversion MatrixEach cell writes the source as the target and reads it back. The symbol is the worst finding. ⚪ exact 🟢 lossless 🔵 restated: only values the target requires were synthesized 🟡 approximate: values changed to fit 🟠 dropped: source data has no target field 🔴 failed: error, invariant, or baseline change Transmission: into a case format
Transmission: into an exchange format
Transmission: into a dataset directory
Distribution: into a distribution format
Full warning details: download |
|
@ferrandor just invited you to this repo to review this PR. Thanks for your feedback on everything. would be great to get this reviewed when you can. I think the key thing you might like here is that RAVENS is now embedded in the @MohamedNumair just wanted to make you aware of this, your commits are cherry picked and preserved. Please feel free to review if you get the chance (also we should catch up in Boston!). Thanks you for your help with everything. I'll mark this PR ready for review shortly. |
frederikgeth
left a comment
frederikgeth
left a comment
There was a problem hiding this comment.
Sam, I reviewed head 4f3cee98. I’m requesting changes for these five correctness issues. The 27 relevant existing tests pass, but each issue below has a failing reproduction.
-
[P1] Restore the neutral when importing Kron-reduced wye transformers.
Inpmd/math/read.rs:801, the reader takes the reduced winding connections directly intoDistWinding.Reproduction: Parse the existing
ut_trans_2w_yy_math.jsonfixture, emit it aspmd-json, and parse that output. The resulting transformer has two phases, while the corresponding ENGINEERING fixture has three. The winding map is[1,2,3]instead of[1,2,3,4].Please restore the grounded wye return at the winding and bus, as the load reader already does for an eliminated neutral.
-
[P1] Preserve out-of-service status in the multiconductor MG-RAVENS reader.
ravens/read.rs:610does not retainEquipment.inService.Reproduction: Set
Equipment.inService=falseon theACLineSegmentobjects inravens/case3_balanced.json, parse the document, then emitpmd-json. Both lines are written withstatus="ENABLED".This silently changes the outage topology. Please retain and propagate the status; no diagnostic currently reports its loss.
-
[P2] Keep source impedance when only resistance or reactance is nonzero.
pmd/math/write.rs:1560requires both matrices to be nonzero before creating the Thevenin branch.Reproduction: Parse
pmd/case3_balanced.json, set the source’srsmatrix to zero andxsto an identity matrix, then emitpmd-math-json. No virtual source branch is emitted; the source instead fixes the terminal voltage directly.The condition should use
||, retaining a purely reactive or purely resistive impedance. -
[P2] Preserve the neutral connection on four-wire switches.
Inravens/write.rs:702,hot_coderemoves the neutral. When the endpoint maps are identical, noSwitchPhaserecords are emitted either.Reproduction: Add a closed switch with
[1,2,3,4]at both ends, canonically emit MG-RAVENS, and parse it again. Both maps become[1,2,3], with no warning.Please preserve the switched neutral in both emission and readback.
-
[P2] Respect line-specific current limits.
ravens/write.rs:620reads ampacity only from the linecode.Reproduction: In the MG-RAVENS fixture, set a line’s
i_maxto[100,100,100], leaving its linecode at[600,600,600]. Canonical emission and readback return the 600 A limit, with no warning.Please give
line.i_maxprecedence over the linecode default.
These reproductions exercise canonical emission or cross-format conversion, bypassing the retained-source echo.
To run them, save the complete file below as powerio-dist/tests/review_regressions.rs. It reuses this branch’s existing tests/helpers module and fixtures; no additional test data is needed. From the repository root run:
cargo test -p powerio-dist --test review_regressions -- --nocaptureOn the reviewed head the result is 0 passed, 5 failed, matching the five issues above.
Complete reproduction test file: powerio-dist/tests/review_regressions.rs
mod helpers;
use powerio_dist::{DistSwitch, DistTargetFormat};
use serde_json::{Value, json};
use std::path::PathBuf;
fn case(rel: &str) -> PathBuf {
PathBuf::from(env!("CARGO_MANIFEST_DIR"))
.join("../tests/data/dist")
.join(rel)
}
#[test]
fn ravens_preserves_line_rating_override() {
let mut net =
helpers::parse_file(case("ravens/case3_balanced.json"), Some("ravens-json")).unwrap();
net.lines_mut()[0].i_max = Some(vec![100.0; 3]);
let out = net.emit_value(DistTargetFormat::RavensJson);
let back = helpers::parse_str(&out.text, "ravens-json").unwrap();
let line = back
.lines()
.iter()
.find(|l| l.name == net.lines()[0].name)
.unwrap();
let limit = line
.i_max
.as_ref()
.or(back.linecode(&line.linecode).unwrap().i_max.as_ref());
assert_eq!(limit, Some(&vec![100.0; 3]), "warnings: {:?}", out.warnings);
}
#[test]
fn ravens_preserves_switch_neutral() {
let mut net =
helpers::parse_file(case("ravens/case3_balanced.json"), Some("ravens-json")).unwrap();
let terminals = vec!["1".into(), "2".into(), "3".into(), "4".into()];
net.switches_mut().push(DistSwitch::new(
"fourwire",
"sourcebus",
"loadbus",
terminals.clone(),
terminals.clone(),
false,
));
let out = net.emit_value(DistTargetFormat::RavensJson);
let back = helpers::parse_str(&out.text, "ravens-json").unwrap();
assert_eq!(
back.switches()[0].terminal_map_from,
terminals,
"warnings: {:?}",
out.warnings
);
}
#[test]
fn ravens_reads_disabled_line_as_disabled() {
let text = std::fs::read_to_string(case("ravens/case3_balanced.json")).unwrap();
fn disable(v: &mut Value) {
match v {
Value::Object(o) => {
if o.get("Ravens.cimObjectType").and_then(Value::as_str) == Some("ACLineSegment") {
o.insert("Equipment.inService".into(), json!(false));
}
for child in o.values_mut() {
disable(child);
}
}
Value::Array(a) => {
for child in a {
disable(child);
}
}
_ => {}
}
}
let mut doc: Value = serde_json::from_str(&text).unwrap();
disable(&mut doc);
let net = helpers::parse_str(&doc.to_string(), "ravens-json").unwrap();
let out = net.emit_value(DistTargetFormat::PmdJson);
let doc: Value = serde_json::from_str(&out.text).unwrap();
assert!(
doc["line"]
.as_object()
.unwrap()
.values()
.all(|line| line["status"] == "DISABLED"),
"out-of-service lines became enabled: {}; read warnings: {:?}; emit warnings: {:?}",
doc["line"],
net.warnings,
out.warnings
);
}
#[test]
fn pmd_math_preserves_purely_reactive_source_impedance() {
let mut net = helpers::parse_file(case("pmd/case3_balanced.json"), Some("pmd-json")).unwrap();
let n = net.sources()[0].terminal_map.len();
net.sources_mut()[0]
.extras
.insert("rs".into(), json!(vec![vec![0.0; n]; n]));
let xs: Vec<Vec<f64>> = (0..n)
.map(|i| (0..n).map(|j| if i == j { 1.0 } else { 0.0 }).collect())
.collect();
net.sources_mut()[0].extras.insert("xs".into(), json!(xs));
let out = net.emit_value(DistTargetFormat::PmdMathJson);
let doc: Value = serde_json::from_str(&out.text).unwrap();
let virtual_branches: Vec<_> = doc["branch"]
.as_object()
.unwrap()
.values()
.filter(|b| {
b["name"]
.as_str()
.unwrap()
.starts_with("_virtual_branch.voltage_source.")
})
.collect();
assert_eq!(
virtual_branches.len(),
1,
"source reactance must not become zero; warnings: {:?}",
out.warnings
);
}
#[test]
fn pmd_math_restores_kron_reduced_wye_transformer_neutral() {
let eng =
helpers::parse_file(case("pmd/math/ut_trans_2w_yy_eng.json"), Some("pmd-json")).unwrap();
let math = helpers::parse_file(
case("pmd/math/ut_trans_2w_yy_math.json"),
Some("pmd-math-json"),
)
.unwrap();
let out = math.emit_value(DistTargetFormat::PmdJson);
let back = helpers::parse_str(&out.text, "pmd-json").unwrap();
assert_eq!(
back.transformers()[0].phases,
eng.transformers()[0].phases,
"MATHEMATICAL -> ENGINEERING must not turn a three phase transformer into two phases"
);
for (e, m) in eng.transformers()[0]
.windings
.iter()
.zip(&math.transformers()[0].windings)
{
assert_eq!(
m.terminal_map, e.terminal_map,
"Kron reduced wye windings require their grounded return; math diagnostics: {:?}",
math.warnings
);
}
}
frederikgeth
left a comment
frederikgeth
left a comment
There was a problem hiding this comment.
Sam, I re-reviewed head 8e30071 against the previous five findings. Three are addressed:
- Kron-reduced wye transformers retain their grounded neutral and three-phase identity.
- Four-wire switches preserve the neutral connection.
- Inactive MG-RAVENS equipment stays outside the active network, is retained for canonical MG-RAVENS emission, and is diagnosed when another format drops it.
I updated the inactive-equipment reproduction to accept this implementation. It checks the original disabled components by name, so the newly generated, legitimately active source-impedance branch does not cause a false failure.
I’m keeping request changes for the two remaining issues:
-
[P2] Purely reactive or purely resistive source impedance still disappears.
powerio-dist/src/pmd/math/write.rs:1669still usesnonzero(&rs) && nonzero(&xs)in the voltage-source writer.Reproduction: Parse
tests/data/dist/pmd/case3_balanced.json, set the source’srsmatrix to zero andxsto an identity matrix, then emitpmd-math-json. The output contains zero virtual source branches, although the source has nonzero reactance. Its terminal voltage is fixed directly instead, with no diagnostic reporting the lost impedance.Please create the Thevenin branch when either matrix is nonzero (
||). The existing||in the switch writer does not cover this source path. -
[P2] Line-specific current limits are still ignored by MG-RAVENS emission.
powerio-dist/src/ravens/write.rs:712still obtains emergency ampacity only fromlinecode.i_max.Reproduction: Set a line’s
i_maxto[100,100,100]in the MG-RAVENS fixture, leaving its linecode at[600,600,600]. Canonical MG-RAVENS emission and readback return 600 A, with no emission warnings.Please give
line.i_maxprecedence over the linecode default.
Verification: 958 existing tests passed (three ignored). The updated five reproductions report 3 passed, 2 failed, with only the two tests above failing. These exercise canonical emission or cross-format conversion, bypassing retained-source echo.
Save the complete file below as powerio-dist/tests/review_followup.rs. It reuses this branch’s existing tests/helpers module and fixtures; no additional test data is needed. Run from the repository root:
cargo test -p powerio-dist --test review_followup -- --nocaptureExpected result on 8e30071:
pmd_math_restores_kron_reduced_wye_transformer_neutral ... ok
ravens_preserves_switch_neutral ... ok
ravens_reads_disabled_line_as_disabled ... ok
pmd_math_preserves_purely_reactive_source_impedance ... FAILED
ravens_preserves_line_rating_override ... FAILED
test result: FAILED. 3 passed; 2 failed
Complete updated reproduction file: powerio-dist/tests/review_followup.rs
mod helpers;
use powerio_dist::{DistSwitch, DistTargetFormat};
use serde_json::{Value, json};
use std::path::PathBuf;
fn case(rel: &str) -> PathBuf {
PathBuf::from(env!("CARGO_MANIFEST_DIR"))
.join("../tests/data/dist")
.join(rel)
}
#[test]
fn ravens_preserves_line_rating_override() {
let mut net =
helpers::parse_file(case("ravens/case3_balanced.json"), Some("ravens-json")).unwrap();
net.lines_mut()[0].i_max = Some(vec![100.0; 3]);
let out = net.emit_value(DistTargetFormat::RavensJson);
let back = helpers::parse_str(&out.text, "ravens-json").unwrap();
let line = back
.lines()
.iter()
.find(|l| l.name == net.lines()[0].name)
.unwrap();
let limit = line
.i_max
.as_ref()
.or(back.linecode(&line.linecode).unwrap().i_max.as_ref());
assert_eq!(limit, Some(&vec![100.0; 3]), "warnings: {:?}", out.warnings);
}
#[test]
fn ravens_preserves_switch_neutral() {
let mut net =
helpers::parse_file(case("ravens/case3_balanced.json"), Some("ravens-json")).unwrap();
let terminals = vec!["1".into(), "2".into(), "3".into(), "4".into()];
net.switches_mut().push(DistSwitch::new(
"fourwire",
"sourcebus",
"loadbus",
terminals.clone(),
terminals.clone(),
false,
));
let out = net.emit_value(DistTargetFormat::RavensJson);
let back = helpers::parse_str(&out.text, "ravens-json").unwrap();
assert_eq!(
back.switches()[0].terminal_map_from,
terminals,
"warnings: {:?}",
out.warnings
);
}
#[test]
fn ravens_reads_disabled_line_as_disabled() {
let text = std::fs::read_to_string(case("ravens/case3_balanced.json")).unwrap();
fn disable(v: &mut Value) {
match v {
Value::Object(o) => {
if o.get("Ravens.cimObjectType").and_then(Value::as_str) == Some("ACLineSegment") {
o.insert("Equipment.inService".into(), json!(false));
}
for child in o.values_mut() {
disable(child);
}
}
Value::Array(a) => {
for child in a {
disable(child);
}
}
_ => {}
}
}
let mut doc: Value = serde_json::from_str(&text).unwrap();
disable(&mut doc);
let net = helpers::parse_str(&doc.to_string(), "ravens-json").unwrap();
let out = net.emit_value(DistTargetFormat::PmdJson);
let doc: Value = serde_json::from_str(&out.text).unwrap();
for name in ["ohline", "quad"] {
// The revised reader retains inactive equipment untyped and omits
// it from active topology. Unrelated generated source branches can
// legitimately remain enabled.
let line = &doc["line"][name];
assert!(line.is_null() || line["status"] == "DISABLED");
assert!(!net.lines().iter().any(|line| line.name == name));
assert!(net.untyped_objects().iter().any(|object| object.class == "ACLineSegment" && object.name == name));
assert!(out.warnings.iter().any(|warning| warning.contains(name) && warning.contains("RECORD_DROPPED")));
}
let canonical = net.emit_value(DistTargetFormat::RavensJson);
let canonical: Value = serde_json::from_str(&canonical.text).unwrap();
fn collect_inactive(value: &Value, found: &mut Vec<String>) {
match value {
Value::Object(object) => {
if object.get("Ravens.cimObjectType").and_then(Value::as_str) == Some("ACLineSegment")
&& object.get("Equipment.inService") == Some(&json!(false)) {
found.push(object["IdentifiedObject.name"].as_str().unwrap().to_owned());
}
for child in object.values() { collect_inactive(child, found); }
}
Value::Array(values) => for child in values { collect_inactive(child, found); },
_ => {}
}
}
let mut found = Vec::new();
collect_inactive(&canonical, &mut found);
assert!(found.iter().any(|name| name == "ohline"));
assert!(found.iter().any(|name| name == "quad"));
}
#[test]
fn pmd_math_preserves_purely_reactive_source_impedance() {
let mut net = helpers::parse_file(case("pmd/case3_balanced.json"), Some("pmd-json")).unwrap();
let n = net.sources()[0].terminal_map.len();
net.sources_mut()[0]
.extras
.insert("rs".into(), json!(vec![vec![0.0; n]; n]));
let xs: Vec<Vec<f64>> = (0..n)
.map(|i| (0..n).map(|j| if i == j { 1.0 } else { 0.0 }).collect())
.collect();
net.sources_mut()[0].extras.insert("xs".into(), json!(xs));
let out = net.emit_value(DistTargetFormat::PmdMathJson);
let doc: Value = serde_json::from_str(&out.text).unwrap();
let virtual_branches: Vec<_> = doc["branch"]
.as_object()
.unwrap()
.values()
.filter(|b| {
b["name"]
.as_str()
.unwrap()
.starts_with("_virtual_branch.voltage_source.")
})
.collect();
assert_eq!(
virtual_branches.len(),
1,
"source reactance must not become zero; warnings: {:?}",
out.warnings
);
}
#[test]
fn pmd_math_restores_kron_reduced_wye_transformer_neutral() {
let eng =
helpers::parse_file(case("pmd/math/ut_trans_2w_yy_eng.json"), Some("pmd-json")).unwrap();
let math = helpers::parse_file(
case("pmd/math/ut_trans_2w_yy_math.json"),
Some("pmd-math-json"),
)
.unwrap();
let out = math.emit_value(DistTargetFormat::PmdJson);
let back = helpers::parse_str(&out.text, "pmd-json").unwrap();
assert_eq!(
back.transformers()[0].phases,
eng.transformers()[0].phases,
"MATHEMATICAL -> ENGINEERING must not turn a three phase transformer into two phases"
);
for (e, m) in eng.transformers()[0]
.windings
.iter()
.zip(&math.transformers()[0].windings)
{
assert_eq!(
m.terminal_map, e.terminal_map,
"Kron reduced wye windings require their grounded return; math diagnostics: {:?}",
math.warnings
);
}
}
Address Frederik Geth's two remaining RAVENS/PMD review findings. Retain either source impedance component, give segment ratings precedence, and preserve distinct ratings across lines sharing a linecode. Commit all five review reproductions, boundary-case regression coverage, and the durable handoff record. The distribution suite, conversion matrix, and all seven Clippy configurations pass locally. Co-authored-by: Frederik Geth <12539926+frederikgeth@users.noreply.github.com>
|
Fred's two remaining requested changes are implemented in 43c6d56, and re-review is requested.
I reproduced the reported 3-pass/2-fail result on The follow-up's full GitHub CI is running; this is not a claim that the new head's entire CI is already green. Julia #197 remains at the approved All fixes, tests and restart instructions are committed on this branch. The durable handoff and fresh-session prompt require no files from this session. No history was rewritten, and nothing was merged or released. |
samtalki
changed the title
frederikgeth
left a comment
frederikgeth
left a comment
There was a problem hiding this comment.
Sam, I re-reviewed head 7a02689e92630833acbbc14697c20dd2e039b8be, focusing on the fixes since my last review and adjacent paths. My previous five findings are addressed, and all nine committed follow-up regressions pass.
I am keeping request changes for two additional boundary cases, detailed in the inline comments:
- [P2] Keep RAVENS segment current limits off the shared linecode. A rated segment gives its 100 A limit to an unrated segment sharing the same linecode, without a diagnostic.
- [P2] Map virtual-source grounding flags to the source connections. A phase-B-only source with nonzero reactance emits
terminals: [2]butgrounded: [false,false,false], producing inconsistent PMD bus arrays.
Verification: All 37 tests in pmd_math, ravens, and review_followup passed. Both additional reproductions below fail on the reviewed head. These use canonical emission, bypassing retained-source echo. The grounding reproduction checks the emitted arrays; I also checked PMD's source, which indexes bus["terminals"] with bus["grounded"]. I did not run a native PMD solve.
Save the following as powerio-dist/tests/review_boundary.rs and run:
cargo test -p powerio-dist --test review_boundary -- --nocaptureExpected result on this head: 0 passed, 2 failed. No additional fixtures are needed.
Complete reproduction test file
mod helpers;
use powerio_dist::DistTargetFormat;
use serde_json::{Value, json};
use std::path::PathBuf;
fn case(rel: &str) -> PathBuf {
PathBuf::from(env!("CARGO_MANIFEST_DIR"))
.join("../tests/data/dist")
.join(rel)
}
#[test]
fn source_impedance_virtual_bus_grounding_matches_source_connections() {
let mut net = helpers::parse_file(case("pmd/case3_balanced.json"), Some("pmd-json")).unwrap();
let source = &mut net.sources_mut()[0];
source.terminal_map = vec!["2".into()];
source.v_magnitude = vec![source.v_magnitude[1]];
source.v_angle = vec![source.v_angle[1]];
source.extras.insert("rs".into(), json!([[0.0]]));
source.extras.insert("xs".into(), json!([[1.0]]));
let out = net.emit_value(DistTargetFormat::PmdMathJson);
let doc: Value = serde_json::from_str(&out.text).unwrap();
let bus = doc["bus"]
.as_object()
.unwrap()
.values()
.find(|b| {
b["name"]
.as_str()
.unwrap()
.starts_with("_virtual_bus.voltage_source.")
})
.unwrap();
assert_eq!(bus["terminals"], json!([2]));
assert_eq!(
bus["grounded"],
json!([false]),
"virtual bus: {bus}; warnings: {:?}",
out.warnings
);
}
#[test]
fn ravens_unrated_line_does_not_inherit_another_segments_limit() {
let mut net =
helpers::parse_file(case("ravens/case3_balanced.json"), Some("ravens-json")).unwrap();
let mut other = net.lines()[0].clone();
other.name = "zz_unrated".into();
other.extras.clear();
other.i_max = None;
let code_name = other.linecode.clone();
net.line_codes_mut()
.iter_mut()
.find(|c| c.name == code_name)
.unwrap()
.i_max = None;
net.lines_mut()[0].i_max = Some(vec![100.0; 3]);
net.lines_mut().push(other);
let out = net.emit_value(DistTargetFormat::RavensJson);
let back = helpers::parse_str(&out.text, "ravens-json").unwrap();
let line = back
.lines()
.iter()
.find(|l| l.name == "zz_unrated")
.unwrap();
let limit = line
.i_max
.as_ref()
.or(back.linecode(&line.linecode).unwrap().i_max.as_ref());
assert_eq!(
limit, None,
"warnings: {:?}; {:?}",
out.warnings, back.warnings
);
}| match &code.i_max { | ||
| Some(existing) if *existing != proposed => conflicts = true, | ||
| Some(_) => {} | ||
| None => code.i_max = Some(proposed), |
There was a problem hiding this comment.
[P2] Keep segment current limits off the shared linecode. When a segment has an emergency limit and the linecode has none, this assignment makes that segment's limit the default for every segment sharing the code. Reproduction: start from ravens/case3_balanced.json, clear the first line's linecode rating, give that line a [100,100,100] override, and clone it as zz_unrated with no override or normamps. Canonical emission and readback give zz_unrated a 100 A effective limit, although it had none; no diagnostic reports this change. The new conflict handling preserves distinct explicit limits, but does not protect unrated segments. Please retain each segment's limit in line.i_max without synthesizing a shared default from it. The complete failing reproduction is in the review body.
| .and_then(Value::as_array) | ||
| .map_or_else( | ||
| || vec![false; nconductors], | ||
| |a| a.iter().map(|v| v.as_bool().unwrap_or(false)).collect(), |
There was a problem hiding this comment.
[P2] Map grounding flags to the source's connections. This copies every grounding flag from the parent bus, while the virtual bus's terminals contains only the source connections. Reproduction: in pmd/case3_balanced.json, restrict the source to phase B (terminal_map = ["2"]), keep its corresponding voltage phasor, and use rs=[[0]], xs=[[1]]. The output virtual bus has terminals: [2] and grounded: [false,false,false]. PMD uses the grounding array as a Boolean index into the terminal array, so these dimensions are incompatible. Please select and order the parent grounding flags by the source connection IDs before setting the virtual bus's grounding and voltage bounds. The complete failing reproduction is in the review body.
MohamedNumair
left a comment
MohamedNumair
left a comment
There was a problem hiding this comment.
Thanks Sam, and thanks for carrying my RAVENS commits across, really happy to collaborate on these amazing projects and would love to meet up!
Regarding this PR I did a deeper review with my old fork. The shared powerio-core::cim vocabulary, the profile dispatch in powerio-tnd, and the PMD MATHEMATICAL oracle fixtures are a big step up from where my branches left off. I reviewed the whole head (7a02689), with most attention on the multiconductor RAVENS path, how it interoperates with cim-dist, and how it reads the upstream MG-RAVENS data. (I thought its a clever way to make the AI-review verifiable against what I have been working on in cim)
I'd like the items under "Blocking" settled before this merges. Fixes for the first group are already written and tested; you can cherry-pick them again from my branches mentioned below
Below is what could be useful for an agent continuing on this...
Fixes ready to cherry-pick
Branch ravens-review-fixes on my fork, three commits on top of 7a02689:
git fetch https://github.com/MohamedNumair/powerio ravens-review-fixes
git cherry-pick ba0bfca3 c331ddbc 5c6b8e65
ba0bfca3: Fred's two remaining boundary cases. His reproductions are committed as powerio-dist/tests/review_boundary.rs (0/2 before, 2/2 after).
ravens/read.rs: a segment's emergency limit stays on the line. It is no longer copied into the shared linecode as a default. MG-RAVENS rates segments, not impedances, so this is the representation without the leak. I updatedtests/ravens.rsand the fallback case ofravens_line_rating_precedence_and_fallback, which now states its own linecode default.pmd/math/write.rs: the virtual source bus takes itsgroundedflags from the parent bus terminals that the source actually connects to, in connection order.
c331ddbc: carry feeders between cim-dist and MG-RAVENS intact. It adds powerio-dist/tests/ravens_cim.rs (8 tests).
- mRIDs. Both formats carry
IdentifiedObject.mRIDon the same CIM classes, yet CIM → RAVENS → CIM regenerated every id. Each writer also reported the other's id as an "extra with no slot". Now each writer reuses the other's key, and CIM → RAVENS → CIM keeps every non-transformer id. Transformers are excluded deliberately: RAVENS keys them by the enclosingPowerTransformer, which several CIM tanks can share. - Open-delta tanks (P1, silent). A delta end on two conductors (
AB) was counted as two phases.tank_feeder.xml'sod_t1/od_t2came back withphases = 2, which halves the per-phase kVA and moves the primary onto a line-to-neutral base (ratio 0.578 → 1.001, about a √3 error), with no warning. This was my bug, from4a62a25. - The upstream layout. At the pinned MG-RAVENS revision (
094ad116),examples/IEEE13_Assets.jsonstates transformer terminals once, on thePowerTransformer, and names the regulated end withTransformerEnd.RatioTapChanger.opendss2xml.py:1677writes it that way, and PMD'sravens2math.jlreads terminals from thePowerTransformertoo. The reader dropped all five tanks. Once they were read, it would have put each regulator's tap on the source-side winding, becauseneutralUmatches both windings of a 1:1 regulator, which inverts the regulation. Both are now read as written. - Transformer leakage impedance (P1, interop with the LANL tools). We didn't store
ShortCircuitTest.leakageImpedancethe way the MG-RAVENS tools do. Takexfm1from IEEE 13 (x = 2%, r = 0.55% per winding, base impedance 34.6 Ω):- Upstream's converter (
opendss2xml.py:1580) stores the total impedance in percent times the base ohms, resistance included: 2.28 × 34.6 = 79.0. PMD's RAVENS reader (custom-main-ravens,ravens2math.jl:953) undoes exactly that. - We stored the reactance alone in plain ohms: 2 / 100 × 34.6 = 0.69.
- So we read upstream's 79.0 as 228% reactance instead of 2%. PMD, reading our 0.69, would take the square root of a negative number.
- The fix writes and reads the number the way upstream and PMD do. The IEEE 13 example's
sub,xfm1andreg1–reg3now match the OpenDSS values exactly. - This isn't the CIM standard's plain ohms, which
cim-distkeeps. Conversions between the two go through the model, so both stay correct. If you'd rather follow the CIM standard in RAVENS too, that needs a matching change in the LANL tools first.
- Upstream's converter (
5c6b8e65: the upstream IEEE 13 example as an oracle. It vendors examples/IEEE13_Assets.json from MG-RAVENS 094ad116 byte exact as tests/data/dist/ravens/IEEE13_Assets.json (3,358 lines, 160,797 bytes, Apache-2.0 under the existing ravens/License.md, provenance in tests/data/dist/README.md). It is over our 100 KiB fixture guideline; I'm approving it because it's the only upstream multiconductor network besides case3. powerio-dist/tests/ravens_ieee13.rs reads it against opendss/ieee13/IEEE13Nodeckt.dss:
- the five transformers match OpenDSS in phases, connections, ratings,
%randxsc, with each regulator's tap on its tap winding; - the fifteen loads match in bus, configuration and power;
- the ten wire-asset segments (see below) are each named by a finding rather than dropped, so that test fails, and becomes the comparison, when asset impedances land;
- same-format output echoes the upstream bytes and the canonical writer is a fixed point.
evals/validation/ravens/check.py globs dist/ravens/*.json, so the schema gate will now include this file. serialize and the IR → RAVENS emission both succeed on it, but I couldn't run the schema gate itself locally. Please check ravens-schema.yml on the cherry-pick.
Checks run on 5c6b8e65:
cargo test -p powerio-dist: 910 passed.conversion_matrix_report: 14 passed, baseline unchanged.cargo test -p powerio --test ravensand-p powerio-tndpass.cargo fmt --checkis clean. Clippy (-D warnings) is clean onpowerio-distlib and all touched tests.
Test data: what MG-RAVENS files exist
At 094ad116, upstream ships two network documents: examples/case3_balanced.json (already vendored, but only lines, loads and a source) and examples/IEEE13_Assets.json (now vendored, above). The examples/schema/*.json files are per-class fragments, not networks.
Even with my fixes, IEEE13_Assets.json reads 1 of its 11 segments as a typed line: the other ten state their impedance through wire and cable assets (WireSpacingInfo / OverheadWireInfo / ConcentricNeutralCableInfo / TapeShieldCableInfo), which are not implemented. The PR description and docs/design/ravens-review.md say the readers handle "the data produced by the upstream OpenDSS converter". That is only true for linecode-based feeders, and the docs should say so. A wire-geometry (Carson) impedance step would close it, and the new IEEE 13 test is ready to become the comparison.
The cim-dist ↔ RAVENS tests above use our own cim/micro_feeder.xml, cim/tank_feeder.xml and ravens/case3_balanced.json. Between them they cover open-delta, center-tap, regulator, delta-wye and single-phase tanks.
Blocking (not fixed in my commits)
- RAVENS voltage limits carry no line-to-line/phase conversion (
ravens/read.rs:462, the writer atwrite.rs:327).- The fixture states
VoltageLimit.value380/420 againstnormalValue400 on a 400 V line-to-line base. The reader stores 380/420 as the scalarv_min/v_max, which the PMD writer treats as phase-to-ground. - Reproduced:
ravens/case3_balanced.json→pmd-jsongivesloadbusvm_lb = [0.38, 0.38, 0.38, 0]kV against a 0.2309 kV phase base, a 1.65 pu lower bound. - Please confirm which convention
opendss2xmluses and scale on read and write. No test checks the value today.
- The fixture states
- A split unbalanced source keeps its Thevenin impedance on phase A only (
ravens/write.rs, theSOURCE_SPLITbranch).r/x/r0/x0ride on the first single-phase part, so B and C read back as ideal sources. Either refuse to split a source with impedance or write the coupled impedance explicitly, and in either case name it in the diagnostic. - PMD MATHEMATICAL non-Kron transformer lowering is miswired (
pmd/math/write.rs:988,:1051).- Internal and star buses get terminals
1..=nphases(+1), but the loss-model branches connecthot = connections[..nphases]. - A single-phase unit on phase B (
["2","4"]) putsf_connections = [2]on the internal bus's grounded terminal. - This path runs whenever Kron reduction is off, fails, or finds no grounded bus.
- PMD uses
bus["terminals"][1:nphases]there. A non-Kron PMD export fixture would pin it.
- Internal and star buses get terminals
- Balanced RAVENS: a line's
BaseVoltagevotes for both of its buses (powerio-tx/src/format/ravens.rs:1158,:1791).- The writer stamps every
ACLineSegmentwith the from-bus kV. The reader ties that kV to every terminal, and line votes come first. - A
tap = 0branch between buses of different base kV therefore rebases the to-bus. - Reproduced on
case2869pegase.m: after MATPOWER → RAVENS → read, bus 1968 comes back at 220 kV instead of 150 kV. That scales itsvmlimits and the per-unit impedance of its branches. - The read does report "conflicting BaseVoltage values … keeping the first" (34 times on pegase), but it keeps the line's value.
- Let line votes fill only buses that nothing else anchors.
- The writer stamps every
Should fix (P2)
I traced these in the code; I didn't build a reproduction for every one.
- Kron-reduced wye equipment (a load
[1,2,3]or[1], a generator[1,2,3]) comes back with a floating, ungrounded neutral or is dropped as "delta". The writer only states the return when the map has a neutral. Transformer wye ends getNinorderedPhasesbut noGround. s21/s21Nphase codes fail the whole parse (expand_phase_codeskips the1).- Switch subclasses.
BreakerandRecloserare rewritten as plainSwitchwithout a diagnostic.Fuse,Disconnector,SectionaliserandJumperare left untyped, which islands the downstream feeder. - Transformers with zero or infinite
s_ratingloseTransformerEndInfo.rand the short-circuit test on write, so they read back ideal. That contradicts the "positive computational impedance base" note in the review doc. - PMD MATHEMATICAL:
- The Kron-reduced switch keeps its unreduced
rs/xs(the leading block instead ofZ_pp − Z_pn Z_nn⁻¹ Z_np). - There is no delta phase projection for two-terminal delta loads.
vm_pair_lb/ubare stashed on read but written as[]without a finding.- MATHEMATICAL → ENGINEERING ZIP alphas collapse to the first phase even when that phase is zero.
- The Kron-reduced switch keeps its unreduced
- Balanced RAVENS:
Doc::resolvescans every object per lookup, so reads are quadratic in document size (pegase takes about 5 s to read in a debug build). Index it by name once.- A load, shunt, machine, source, line or transformer with a missing terminal is dropped without
RECORD_DROPPED. baseMVA≤ 0 is not rejected.- The
.jsonclassifier (routing.rs:556),detect_profile, andreject_multiconductordisagree on bareAssetInfo/TransformerTankkeys. Making them sharedetect_profilewould settle it.
- Coverage.
matrix.rs:936only compares electrical equivalents for RAVENS when the projection dropped lines or loads. Otherwise RAVENS is checked only for byte idempotence, which is how the open-delta case got through. Runningdifferencesunconditionally would have caught it.
Follow-ups (fine after merge)
cim-disthas noOperationalLimitSetmapping, so RAVENS → CIM drops segment ampacity and bus voltage limits. Both are named findings, and now thatpowerio-core::cimexists the mapping could be shared.cim-distwrites a center tap's second half[4, 2]in A, B, C, N order (named asVALUE_COLLAPSED).TransformerTankEnd.orderedPhases(s2N) could keep the polarity.- The RAVENS writer drops regulator taps with "not synthesized". Now that the reader follows the
RatioTapChangerpointer, the writer could emit one. powerio summaryon a RAVENS document reports"json_format": "bmopf-json".- On the current stable clippy (rustc 1.97),
powerio-dist/tests/pmd_math.rslines 709, 710, 719, 720, 870 and others failclippy::float_cmp. This is unrelated to my commits but will bite once CI's stable moves.
Closes #27. Closes #645.
Adds MG-RAVENS JSON import/export (
ravens-json) and PowerModelsDistribution MATHEMATICAL JSON import/export (pmd-math-json). Mohamed Numair's original commits are preserved. The PMD work follows Robert Ferrando's proposal.RAVENS support
RAVENS can describe a balanced network or list individual phase and neutral conductors. PowerIO reads the data produced by the upstream MATPOWER converter into
BalancedNetwork, and the data produced by the upstream OpenDSS converter intoMulticonductorNetwork. The file contents determine which reader runs.powerio-tx::format::ravenspowerio-dist::ravenspowerio-dist::pmd::mathpowerio-tnd::formats::ravens::parsepowerioparseandemit, supported-format information, and PowerIO IR serializationpowerio-core::cimEach reader stays with the network type it uses. Balanced RAVENS remains usable from
powerio-txwithout a distribution dependency; distribution RAVENS remains usable frompowerio-distwithout a transmission dependency.powerio-tndalready depends on both and also contains SINCAL.Combined transmission/distribution RAVENS import is not implemented. The selection function returns one network type. A file containing record types that require both readers produces
PARSE.RAVENS.MIXED_PROFILE; it does not become anIntegratedNetwork.What to review
The main question is whether reading and writing preserve the electrical data, including after a trip through PowerIO IR that removes the saved source bytes. Tests cover source impedance, equipment status, phase and neutral connections, transformer parameters, and line current limits. Frederik's five reproductions plus four boundary cases are committed in
powerio-dist/tests/review_followup.rs.Known limitations remain visible: balanced RAVENS loses generator voltage setpoints and third branch ratings; distribution conversion reports unsupported conductor identities, additional center-tap leakage pairs, and nonunity winding taps. The archived BMOPF IEEE13
xfm2has ambiguous winding terminals and remains unverified. See the review record for the exact checks and limits.Supporting changes register the formats in the public API and CLI, add regression and external-tool checks, and update the conversion report to show each fixture's field differences. CGMES voltage limits and exchange-format identifier/area diagnostics were fixed where the RAVENS conversion checks exposed losses.
PowerIO IR stays at version 2.
pio-ir/2/0.11.8/schema.jsonadds the new source-format names; published 0.11.7 schema bytes stay unchanged. C ABI 7 remains sufficient. PowerIO.jl #197 is pinned at the approvede5c714dand needs no further code changes.Validation
All 11 GitHub workflows passed at code commit
43c6d56on 2026-10-29. Rust CI passed all nine Frederik follow-up regressions; Julia passed 3,838 tests againste5c714d. Local checks passed 896 distribution tests (18 ignored), all 14 conversion-matrix tests without a baseline change, and all seven Clippy configurations. The subsequent wording change edits only this description and the review document.External checks use pinned MG-RAVENS, native PowSybl, PMD v0.16.0 exports, and OpenDSS. The RAVENS schema check uses the upstream schema unchanged for distribution output. Balanced output requires definitions for two fields emitted by the upstream MATPOWER converter but omitted from its schema; that difference is explicit in the report.
Six independent reviews and their verified fixes are recorded in the handoff. Frederik's re-review is requested. Fixture provenance and licenses are recorded, and every added fixture is below 100 KiB. No merge or release is part of this work.