skyfielders / python-skyfield

Elegant astronomy for Python
MIT License
1.4k stars 211 forks source link

Implement Velocity initialization in m/s #698

Closed facorazza closed 1 year ago

brandon-rhodes commented 2 years ago

For context, could you explain the situation that's led you to need m/s? Over the years I have come to regret my decision to do initialization with big if-then statements, since it leads to several wasted arguments (only one can be used at a time) and a bit of wasted time (it has to step through the if-then), so before expanding one of these further I'll be interested to know the specific use case — the data sources I'd played with so far are all in km/s, and it will help me write more accurate documentation in the future if I learn more about the kinds of projects where m/s measures tend to come from. Thanks for explaining whatever context you are free to provide!

brandon-rhodes commented 2 years ago

An update!

Your suggestion led me to deeply rethink how Skyfield does units constructors. And I've come up with an idea!

https://rhodesmill.org/brandon/2022/a-descriptor-constructor/

Unless anyone warns me that this will spell disaster, I'll plan to make it part of the next release of Skyfield, in which case you'll just be able to type Distance.m(1234) and get the new distance measurement that you need! All will be happy.

facorazza commented 2 years ago

For context, could you explain the situation that's led you to need m/s? Over the years I have come to regret my decision to do initialization with big if-then statements, since it leads to several wasted arguments (only one can be used at a time) and a bit of wasted time (it has to step through the if-then), so before expanding one of these further I'll be interested to know the specific use case — the data sources I'd played with so far are all in km/s, and it will help me write more accurate documentation in the future if I learn more about the kinds of projects where m/s measures tend to come from. Thanks for explaining whatever context you are free to provide!

I'm working on a tool with many different units. As a general rule I'd like to stick with SI units and without using orders of magnitude. Otherwise, I would have to store variables like distance_km or distance_m and perform conversions between them. I might use km for the part using skyfield but, considering that Distance does support initialization in meters, I thought I could simply contribute and align the Velocity initialization.

An update!

Your suggestion led me to deeply rethink how Skyfield does units constructors. And I've come up with an idea!

https://rhodesmill.org/brandon/2022/a-descriptor-constructor/

Unless anyone warns me that this will spell disaster, I'll plan to make it part of the next release of Skyfield, in which case you'll just be able to type Distance.m(1234) and get the new distance measurement that you need! All will be happy.

Yes using @property could easily solve the issue. Unfortunately, method overloading is really hard, if at all possible, in Python. Another solution could be to define units as types and use the decorator @overload.

brandon-rhodes commented 2 years ago

Unfortunately, method overloading is really hard, if at all possible, in Python.

I’m a bit confused, if you could clarify — by “method overloading” do you mean the maneuver that I demonstrated successfully in the article, or do you mean a different technique?

facorazza commented 2 years ago

Unfortunately, method overloading is really hard, if at all possible, in Python.

I’m a bit confused, if you could clarify — by “method overloading” do you mean the maneuver that I demonstrated successfully in the article, or do you mean a different technique?

In many languages it is possible to overload functions like this:

void func(a: int) {...}
void func(a: float) {...}

When the function is called or compiled, it is chosen based on the parameters type passed to it. Hence why a new type for each unit would be needed. It is somewhat similar to the approach you explain in the article and it might not necessarily be better. It was just one thing that came to mind when commenting.

brandon-rhodes commented 2 years ago

Ah — so, yes, if Python had C++-style overloading, and if each unit were a type, then an overloaded constructor function would have been a possible way to implement this, yes.

The reviews of my new idea on Twitter have been favorable — several likes, a couple of positive comments from people I trust, and (so far) no one has any warnings against the approach — so for now I'll plan to make the changes within the next week or so, and then you will be able to try them out and see if the new Distance.m(123.0) solves your use case. I'll comment here once I get the code running in the development version of Skyfield!

brandon-rhodes commented 1 year ago

I finally got around to landing the change! It should appear in the next release of Skyfield, maybe as early as today.