Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a simple transformer effficiency model to pvlib #2053

Merged
merged 55 commits into from
Jun 19, 2024

Conversation

kurt-rhee
Copy link
Contributor

@kurt-rhee kurt-rhee commented May 17, 2024

  • Closes Transformers #1269
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Described in the issue link. Please look out for issues with the rendering of latex in the docstring. I am not a sphinx or latex expert.

@kandersolar kandersolar added this to the 0.11.0 milestone May 20, 2024
Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

@kurt-rhee suggest that we add a new page to pvlib/docs/source/sphinx/reference, call it transformer.rst You can model it on scaling.rst or one of the other simple pages.

To pvlib/docs/source/sphinx/reference/index.rst we would need to add a line. That should get the documents to build.

pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
kurt-rhee and others added 4 commits May 20, 2024 11:34
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
+ added transformer.rst to pvlib/docs/source/sphinx/reference
@kurt-rhee
Copy link
Contributor Author

@cwhanse Thank you for the guidance, I have updated the .rst files. I have one failing check in the PR, but I am not sure how to fix. I develop in Ubuntu 22.04 which may be causing the problem.

Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

Mostly spacing corrections.

Some of my earlier comments were resolved without any change. Was that intentional? We usually include the reference link "[1]_" in this case. Are you opposed to renaming the loss parameters?

docs/sphinx/source/reference/transformer.rst Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
kurt-rhee and others added 9 commits May 20, 2024 15:16
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
@kurt-rhee
Copy link
Contributor Author

Mostly spacing corrections.

Some of my earlier comments were resolved without any change. Was that intentional? We usually include the reference link "[1]_" in this case. Are you opposed to renaming the loss parameters?

Hey Cliff, completely unintentional. I have hopefully committed all proposed changes.

pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
kurt-rhee and others added 4 commits May 20, 2024 15:45
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Copy link
Member

@AdamRJensen AdamRJensen left a comment

Choose a reason for hiding this comment

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

Could we add a link for reference [1] to archive.org. This site seems stable enough to me (I tried to find a DOI).

docs/sphinx/source/whatsnew/v0.11.0.rst Outdated Show resolved Hide resolved
@AdamRJensen
Copy link
Member

@cwhanse I suggest that we still include some kind of notion of AC/DC power for the function parameters. Maybe just in parenthesis. I think this would be rather helpful for newbies giving the function a quick glance. Thoughts?

@cwhanse
Copy link
Member

cwhanse commented May 24, 2024

@cwhanse I suggest that we still include some kind of notion of AC/DC power for the function parameters. Maybe just in parenthesis. I think this would be rather helpful for newbies giving the function a quick glance. Thoughts?

Insert "AC" in the descriptions of input_power and output_power

@echedey-ls
Copy link
Contributor

Transformers don't care which way is in or out. The parallel impedance will consume energy from the grid at night if it stays connected.

My only concern with this function is that @adriesse observation is not documented (can it surprise an user?), and

Note that the positive root must be the correct one if the output power is positive.

may suggest results are being clipped - at least nothing is noted regarding negative output power.

@mikofski
Copy link
Member

Transformers don't care which way is in or out. The parallel impedance will consume energy from the grid at night if it stays connected.

My only concern with this function is that @adriesse observation is not documented (can it surprise an user?), and

Note that the positive root must be the correct one if the output power is positive.

may suggest results are being clipped - at least nothing is noted regarding negative output power.

@echedey-ls as @adriesse indicated it is important for users to know that negative power will be drawn from the grid overnight because the inverters consume night losses while in standby. Maybe we should add a note about this in the fix string. Passing through the xfmr will increase this loss.

We could add a gallery example in a separate PR that illustrates how night losses will draw extra power. Any BESS that charges from the grid ditto will go reverse through the xfmr and lose power. Typically the generation and net power are reported separately because they can be compensated differently in the PPA.

@AdamRJensen I suggest we ask in Google groups if a developer will share a xfmr test report.

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
@echedey-ls
Copy link
Contributor

I suggest we ask in Google groups if a developer will share a xfmr test report.

Gotcha. I'm an intern giving out a hand at an official high voltage test facility here in Spain. I've attached a censored test that can get published so you can double check the information, but both load and all losses are specified under section 3.3 Measurement of short-circuit impedance and load losses

As a sidenote that may be good to note in the function docstring is that EU test are done following UNE 60076.

2023 05 3G 0XXX TRAFO 50 kVA 24 kV.pdf

@AdamRJensen
Copy link
Member

As a sidenote that may be good to note in the function docstring is that EU test are done following UNE 60076.

UNE is the Spanish standardization body, so the European standard is more accurately called EN 60076. This standard derives from an IEC standard of the same number (IEC 60076).

@kurt-rhee The losses in the datasheet are given in W and not relative (%), hence I'm wondering if there is a reason that the losses in the function no_load_loss and load_loss are specified as relative to nominal capacity?

@mikofski
Copy link
Member

mikofski commented Jun 5, 2024

The losses in the datasheet are given in W and not relative (%), hence I'm wondering if there is a reason that the losses in the function no_load_loss and load_loss are specified as relative to nominal capacity?

We have always normalized the loss to the load it was measured at, which is consistent with variable losses as a percentage of load. The fixed or no load value could be given in KVA or MVA.

Another important note when looking at test results is that there are usually multiple hookups at different windings for different transformer ratings. Each has its own test and losses.

@kurt-rhee
Copy link
Contributor Author

Hey @AdamRJensen I am also most familiar with adding loss parameters as a percentage into performance modeling tools. Sorry to everyone that I haven't been more responsive here. I am traveling much of this month, will be back at it sometime in July.

@mikofski
Copy link
Member

Can we resolve the conflicts and merge for next release?

Copy link
Contributor

@echedey-ls echedey-ls left a comment

Choose a reason for hiding this comment

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

LGTM with the optional improvement down below and the other one up in the conversation that changes a +- to \pm in an equation.

Can we resolve the conflicts and merge for next release?

@mikofski and other maintainers, I think @kurt-rhee is on vacation from one of the comments above. I think it will need to be resolved by someone with write permission to this branch.

pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Show resolved Hide resolved
cwhanse and others added 3 commits June 15, 2024 17:33
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
@kandersolar
Copy link
Member

I think a few changes are still needed. I will push up some new commits.

I still wish we could find a proper reference, but at this point I have exhausted my own willingness to invest additional time in locating one. References with actual equations tend to describe models vastly more sophisticated than this one, while "simple" references take the view that $P_{out}$ (i.e. the transformer load) is the known quantity used to calculate $P_{in}$, and of course we want the reverse.

@kandersolar
Copy link
Member

A review by another pair of eyes after this last set of changes would be appreciated!

Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

Just editorial stuff.

pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

a bit more latex consistency

pvlib/transformer.py Outdated Show resolved Hide resolved
pvlib/transformer.py Outdated Show resolved Hide resolved
@kandersolar kandersolar merged commit 418d6d0 into pvlib:main Jun 19, 2024
30 checks passed
@kandersolar
Copy link
Member

Thanks everyone for the collaborative contribution here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transformers
7 participants