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

Temperature Gradient #988

Closed
dglozano opened this issue Nov 16, 2021 · 8 comments
Closed

Temperature Gradient #988

dglozano opened this issue Nov 16, 2021 · 8 comments

Comments

@dglozano
Copy link
Contributor

dglozano commented Nov 16, 2021

Hi 👋🏻 I am using UnitsNet quite a lot and I really love it! So far it has covered most of my use cases, but I recently I got the need to use a TemperatureGradient quantity (https://en.wikipedia.org/wiki/Temperature_gradient).

I know there is LapseRate quantity which is also TemperatureDelta / Length, but it seems to be "semantically" focused on the atmospheric temperature variation and it supports only the DegreesPerKilometer unit.

I was wondering if adding a new TemperatureGradient is an option? If so, I would be willing to create a PR to add it.

Another option would be to make the LapseRate support more units I guess, but both the name and description of the quantity can be a little misleading.

Related (ish) to #324

@angularsen
Copy link
Owner

angularsen commented Nov 16, 2021

Hi, without knowing this domain I would favor moving everything to TemperatureGradient. It might be more consistent with our other quantities to name it TemperaturePerLength, but it seems TemperatureGradient is a well established quantity and maybe the better choice .

LapseRate has been touched in:
#316
#321
#324

@ferittuncer @jnyrup
What do you guys think about merging it into TemperatureGradient (or TemperaturePerLength), and marking LapseRate as obsolete?

There is currently a v5 branch where we allow breaking changes, where we can remove anything made obsolete in v4.
#982

@jnyrup
Copy link
Contributor

jnyrup commented Nov 16, 2021

I'm fine with obsoleting LapseRate as it is a quite meteorology specific unit and a special case of a temperature gradient.

It corresponds to the vertical component of the spatial gradient of temperature
https://en.wikipedia.org/wiki/Lapse_rate

Some observations regard naming.
UnitsNet has several PerLength units and only a single Gradient unit (ElectricCurrentGradient).
Googling a bit it seems TemperatureGradient is an established term.
UnitsNet has ForcePerLength for Surface Tension.

@angularsen
Copy link
Owner

@dglozano It seems we agree then, please go ahead with the PR:

  • Obsolete LapseRate (Add "ObsoleteText": "Use TemperatureGradient instead." on the quantity in LapseRate.json and make sure an obsolete attribute is added to the generated LapseRate struct afterwards.)
  • Add TemperatureGradient with your units plus LapseRate's units
  • Copy any usages of LapseRate: https://github.com/angularsen/UnitsNet/search?q=lapserate
    • Seems used in UnitsNet/CustomCode/Quantities/TemperatureDelta.extra.cs

@dglozano
Copy link
Contributor Author

Have created a PR to implement the suggested changes 😄

Not sure how to copy the / operation for TemperatureDelta/Length without making a breaking change by removing the old one that gives a LapseRate as result.

Any suggestion on how to procede?

image

@angularsen
Copy link
Owner

Oh, that's right. Hm. I propose we allow that breaking change, since those updating will get a compile error (safe) and have an obsolete warning instructing them what to do.

@0xferit
Copy link
Contributor

0xferit commented Nov 22, 2021

Hi, without knowing this domain I would favor moving everything to TemperatureGradient. It might be more consistent with our other quantities to name it TemperaturePerLength, but it seems TemperatureGradient is a well established quantity and maybe the better choice .

LapseRate has been touched in: #316 #321 #324

@ferittuncer @jnyrup What do you guys think about merging it into TemperatureGradient (or TemperaturePerLength), and marking LapseRate as obsolete?

There is currently a v5 branch where we allow breaking changes, where we can remove anything made obsolete in v4. #982

Hello @angularsen, I don't see a problem with this but my opinion might not be so helpful since I'm out of sync for since last 3 years.

@angularsen
Copy link
Owner

No problem @ferittuncer , I just thought I recognized your name in one of the PRs. It helps to know that you don't mind the change, anyhow 👍

@dglozano
Copy link
Contributor Author

Solved in #991 , so I am closing this 😁

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

No branches or pull requests

4 participants