Skip to content

Add NominalPower#1644

Open
upcu wants to merge 2 commits intoangularsen:masterfrom
upcu:add-nominal-power
Open

Add NominalPower#1644
upcu wants to merge 2 commits intoangularsen:masterfrom
upcu:add-nominal-power

Conversation

@upcu
Copy link
Copy Markdown

@upcu upcu commented Jan 14, 2026

@upcu upcu closed this Feb 16, 2026
@upcu upcu reopened this Feb 16, 2026
Copy link
Copy Markdown
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! A couple of things need to be fixed in NominalPower.json:

1. Missing BaseDimensions

NominalPower is dimensionally equivalent to Power (W = kg·m²/s³). The quantity definition needs BaseDimensions, same as Power.json:

"BaseDimensions": {
  "L": 2,
  "M": 1,
  "T": -3
}

2. Missing BaseUnits on WattPeak

The WattPeak unit needs BaseUnits to map to SI base units, same as Power.json's Watt:

"BaseUnits": {
  "L": "Meter",
  "M": "Kilogram",
  "T": "Second"
}

Please add these and the PR should be good to go!

private void GenerateConversionMethods()
{
Writer.WL($@"
#region Conversion Methods
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please don't change generator code unless intentional

@@ -0,0 +1,25 @@
{
"Name": "NominalPower",
Copy link
Copy Markdown
Owner

@angularsen angularsen Feb 27, 2026

Choose a reason for hiding this comment

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

Should we rename this to disambiguate this from other kinds of nominal power?
It seems maybe a bit ambiguous: https://en.wikipedia.org/wiki/Nominal_power

For example, NominalPowerPhotovoltaic

@github-actions
Copy link
Copy Markdown
Contributor

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@claude
Copy link
Copy Markdown

claude Bot commented May 2, 2026

Code Review

Adding NominalPower quantity (WattPeak + SI prefixes) and fixing NanoFramework generator indentation.

Conceptual concern — is NominalPower a distinct quantity?

NominalPower (Wp) is dimensionally identical to Power (W). The "peak" designation is a measurement convention (conditions under STC — Standard Test Conditions for PV), not a different physical quantity. UnitsNet already has Power with Watt and all SI prefixes (Kilowatt, Megawatt, etc.).

Adding a separate quantity for this is unusual. The library currently avoids quantities that are just "power measured in a specific context." Could this instead be handled as additional units in the existing Power quantity (e.g., WattPeak as an alias with a 1:1 conversion factor)?

If there is a reason they must stay separate (e.g., preventing accidental addition of peak-watts and thermal-watts), please explain it in the PR description.

Code generator change bundled in same PR

The large diff (~12k additions / ~9k deletions) is mostly generated code regenerated after fixing indentation in CodeGen/Generators/NanoFrameworkGen/QuantityGenerator.cs. This generator whitespace fix is unrelated to NominalPower. Please separate them into two PRs — one for the generator fix and one for the new quantity. Mixing them makes the NominalPower diff very hard to review.

Unit definition

  • Only one base unit type (WattPeak) with SI prefixes — no other unit systems (e.g., no horsepower-peak equivalent). This is fine if the quantity is PV-specific, but reinforces that this is domain-specific.
  • Missing XmlDocRemarks with a reference URL (the description has the Wikipedia link but it should be in the JSON XmlDocRemarks field so it ends up in the generated XML doc).

Recommendation

  1. Clarify whether this should be a separate quantity or new units in Power.
  2. Split the generator indentation fix into a separate PR.
  3. Add a XmlDocRemarks URL to the JSON definition.

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.

2 participants