Skip to content

clarify units for U1 and Uv temperature model coeff #960

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

Merged
merged 6 commits into from
May 22, 2020

Conversation

mikofski
Copy link
Member

@mikofski mikofski commented Apr 30, 2020

  • Closes #xxxx
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst 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 and Milestone are assigned to the Pull Request and linked Issue.

* closes pvlib#945 
* group denominator together, same way it was done for U0 and Uc, ie: `W/(m^2 C)` instead of `(W/m^2 C)` which is ambiguous because it could mean `W/m^2 * C = W * C / m^2` which is not the same thing
* add missing division symbol before 2nd denominator `(m/s)` to get `W / (m^2 C) / (m/s)` instead of `(W/m^2 C) (m/s)` which looks like `WmC/ (m^2 s)` so it's ambiguous
* pvsyst help docs (https://www.pvsyst.com/help/thermal_loss.htm) say  `W/m²·k / m/s` which is also ambiguous but you can't win em all
@kandersolar
Copy link
Member

In this and other cases with combined units, I wonder if it might be better to use a :math: tag to improve the rendering. The plaintext display is a little difficult to parse:

image

@mikofski
Copy link
Member Author

mikofski commented May 2, 2020

In this and other cases with combined units, I wonder if it might be better to use a :math: tag to improve the rendering.

Everywhere? what about units that are simpler to parse? Probably best to be consistent, right?

Ambient dry bulb temperature [C].

I'm okay with making the shift to use :math: to render all units, but maybe that should be a separate issue? It would be a bit of an undertaking, good first timer maybe?

@cwhanse
Copy link
Member

cwhanse commented May 4, 2020

Everywhere? what about units that are simpler to parse? Probably best to be consistent, right?

Perhaps in the few instances where the unit involves a compound fraction. I'm -1 for using :math: for units otherwise.

@mikofski
Copy link
Member Author

mikofski commented May 9, 2020

Quick poll, so we've decided to use :math: to render these ugly confusing units here only, EG:

in :eq:`pvsyst` :math:`\left[\frac{W/{\left(m^2 C\right)}}{m/s}\right]`.

Looks like this
latex

@cwhanse cwhanse self-requested a review May 13, 2020 01:24
@adriesse
Copy link
Member

Could we do:

 W/m2
------
C(m/s)

This would be more easily associated with the variables going in.

@mikofski
Copy link
Member Author

Could we do: W/m^2 / (C(m/s))

I'm fine with

:math:`\left[\frac{W/{m^2 }}{C \left( m/s \right )}\right]`

which looks like this:
LaTeX equation
Powered by CodeCogs

But this makes me wonder if the U0 and U_c should also be changed to look the same?
LaTeX equation
Powered by CodeCogs

What do y'all think?

@mikofski
Copy link
Member Author

Also should we be using Kelvin instead of Celsius?

@adriesse
Copy link
Member

Sure, but I'd also be ok with the plain text:[W/m^2/C] and [W/m^2/C/(m/s)].
My preference is for Celsius.

@cwhanse
Copy link
Member

cwhanse commented May 20, 2020

My preference is for Celsius.

+1

mikofski added 2 commits May 21, 2020 16:51
* update what's new for v0.8.0
* use textrm for units

Signed-off-by: Mark Mikofski <[email protected]>
@mikofski
Copy link
Member Author

mikofski commented May 22, 2020

sorry stickler but inline math don't render if it's on two lines 😕

@mikofski
Copy link
Member Author

What do you think @CameronTStark? Is this good?

@CameronTStark CameronTStark merged commit e04d959 into pvlib:master May 22, 2020
@CameronTStark
Copy link
Contributor

LGTM @mikofski, thanks!

@mikofski mikofski deleted the patch-1 branch May 22, 2020 19:31
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.

BUG:units for Uv & U1, convective thermal coefficient for Faiman and PVsyst, m/s should be in denominator
5 participants