Refactor Angle as a single class #13

rhannequin · 2024-02-18T17:17:39Z

Previously, there was three subclasses of Angle:

Degree
Radian
Hour

This was not a good approach for the universal notion of what an angle is. The same angle should not be represented by two different objects depending on their unit.

Now, only Angle remains. An angle is always stored with a radian unit, as it is the unit of angle in the International System of Units.

Converting an angle is not about changing its type anymore, but only converting its value from one unit to another. The value is nos accessible through the unit name:

angle = Astronoby::Angle.as_degrees(180)

angle.degrees # => 0.18e3
angle.radians # => 0.31415926...e1

Because angles in degrees and hours are immediately converted, some precision is immediately lost. However, the conversion is done with BigDecimal numbers with a precision of 14. I believe this is more than enough affordable precision loss for this project.

The reader may notice some test values have changed. Because I took the opportunity to remove the ceil method on the angle initialization, precision has actually increased. This change is barely noticeable because it affects far digits of the arc second.
In the future, I will conduct performance tests to figure out if ceiling angles increase the library overall performance and speed of calculations.

Angles can still be initialized from different units with the following class methods:

::as_degrees
::as_radians
::as_hours

String formatting has also changed and can be done immediately on the angle without the need to convert it to another unit before:

angle = Astronoby::Angle.as_degrees(180)
angle.str(:dms)

I will work on making Angle a better value object in the future, to make its use even more convenient and efficient.

Thanks to @JoelQ for inspiring me into this work.

@JoelQ

Previously, there was three subclasses of `Angle`: - `Degree` - `Radian` - `Hour` This was not a good approach for the universal notion of what an angle is. The same angle should not be represented by two different objects depending on their unit. Now, only `Angle` remains. An angle is always stored with a radian unit, as it is the unit of angle in the International System of Units. Converting an angle is not about changing its type anymore, but only converting its value from one unit to another. The value is nos accessible through the unit name: ```rb angle = Astronoby::Angle.as_degrees(180) angle.degrees # => 0.18e3 angle.radians # => 0.31415926... ``` Because angles in degrees and hours are immediately converted, some precision is immediately lost. However, the conversion is done with BigDecimal numbers with a precision of 14. I believe this is more than enough affordable precision loss for this project. The reader may notice some test values has changed. Because I took the opportunity to remove the `ceil` method on the angle initialization, precision has actually increased. This change is barely noticeable because it affects far digits of the arc second. In the future, I will conduct performance tests to figure out if ceiling angles increase the library overall performance and speed of calculations. Angles can still be initialized from different units with the following class methods: - `::as_degrees` - `::as_radians` - `::as_hours` String formatting has also changed and can be done immediately on the angle without the need to convert it to another unit before: ```rb angle = Astronoby::Angle.as_degrees(180) angle.str(:dms) ``` I will work on making `Angle` a better value object in the future, to make its use even more convenient and efficient. Thanks to @JoelQ for inspiring me into this work.

JoelQ

I love this change! I left a bunch of comments but they are mostly out of scope for this PR. Instead, they look forward and what you could build on top of this new Angle class. Once you add a few math operators, I think a lot of your other code starts to get much simpler.

JoelQ · 2024-02-19T14:16:24Z

lib/astronoby/aberration.rb

+          @sun_longitude.radians - @coordinates.longitude.radians
+        ) / Math.cos(@coordinates.latitude.radians) / 3600


Out of scope for this PR but for the future, I wonder if it would be useful to implement some math operators directly on Angle? Then you don't need to unwrap the angle values. Ideally you get to treat Angle instances like numbers, not just wrappers of numbers.

With Angle#cos and Angle#- this method would look like:

(@sun_longitude - @coordinates.longitude).cos / @coordinates.lattitude.cos / 3600

JoelQ · 2024-02-19T14:19:21Z

lib/astronoby/aberration.rb

+        Math.sin(@sun_longitude.radians - @coordinates.longitude.radians) *
+        Math.sin(@coordinates.latitude.radians) / 3600


Similarly here, if you had Angle#sin and Angle#- implemented this method would look like:

(@sun_longitude - @coordinates.logitude).sin * @coordinates.lattitude.sin / 3600

Note that you don't need to implement Angle#* or Angle#/ because Angle#sin would return a float, not an angle.

JoelQ · 2024-02-19T14:20:48Z

lib/astronoby/aberration.rb

-          @sun_longitude.to_radians.value - @coordinates.longitude.to_radians.value
-        ) / Math.cos(@coordinates.latitude.to_radians.value) / 3600
+          @sun_longitude.radians - @coordinates.longitude.radians
+        ) / Math.cos(@coordinates.latitude.radians) / 3600


Out of scope for this PR but I'm wondering with the 3600 number comes from? I'd love to see this as a named constant. Given that I associate the number 360 with degrees in a circle, I'm wondering if this might be a number that needs to be owned by Angle ?

JoelQ · 2024-02-19T14:21:31Z

lib/astronoby/angle.rb

    PRECISION = 14
-    PI = BigMath.PI(10)
+    PI = BigMath.PI(PRECISION)
+    FORMATS = %i[dms hms].freeze


Freeze all the things! ❄️

JoelQ · 2024-02-19T14:24:39Z

lib/astronoby/angle.rb

+      end
+
+      def as_degrees(degrees)
+        radians = degrees / BigDecimal("180") * PI


Thoughts on having a constant like DEGREES_PER_RADIAN = BigDecimal("180") * PI? (I'm not sure I used the correct name)

I chose to add a PI_IN_DEGREES = BigDecimal("180") constant because I wasn't able to add something with PI as its use is different depending on the conversion direction (multiply or divide).

JoelQ · 2024-02-19T14:34:55Z

lib/astronoby/bodies/sun.rb

        longitude: Angle.as_degrees(
-          (true_anomaly.value + longitude_at_perigee.to_degrees.value) % 360
+          (true_anomaly.degrees + longitude_at_perigee.degrees) % 360
        )
      )


Out of scope for this first PR but a future refactor you could do would be to implement some math operators on Angle. Then you don't need to do the unwrap/do math/rewrap dance here. For example, if you implemented Angle#+ and Angle#% this would become

longitude: (true_anomaly + logitude_at_perigee) % 360

The same goes for a lot of the math in this file. By implementing a few arithmetic operators on Angle, you get to work with angle values almost as if they were integers

You will need to pay attention to the types when implementing math operations though. Keep in mind:

Some operations return angles, some return raw integers

Some operations work on two angles, some operations work on an angle and a number

For example Angle#cos would take an angle and return a float. Angle#* is effectively scaling your angle. It takes an angle and a number and returns a new angle.

I'd love to see a future world where this file doesn't do any unwrapping or re-wrapping of angles

lib/astronoby/bodies/sun.rb

JoelQ · 2024-02-19T14:43:41Z

lib/astronoby/coordinates/ecliptic.rb

+        obliquity_in_radians = mean_obliquity.value.radians
+        longitude_in_radians = @longitude.radians
+        latitude_in_radians = @latitude.radians


If you implement math operators on Angle, I think a lot of the complexity here goes away. You would no longer need to care that these angles are expressed in radians. All you care is that you have a series of Angle objects and you can do math on them.

JoelQ · 2024-02-19T14:44:02Z

lib/astronoby/coordinates/ecliptic.rb

        Equatorial.new(
-          right_ascension: right_ascension.to_hours,
-          declination: declination.to_degrees,
+          right_ascension: right_ascension,
+          declination: declination,
          epoch: epoch


I love that the units are now gone!

JoelQ · 2024-02-19T14:45:03Z

lib/astronoby/nutation.rb

+          -17.2 * Math.sin(moon_ascending_node_longitude.radians) -
+          1.3 * Math.sin(2 * sun_mean_longitude.radians)


Out of scope of this PR but there are a bunch of magic numbers that I have not clue what they do. Maybe extract some named constants?

I forgot to answer to this one.
Most of the magic numbers in the library are magic numbers from the book I rely on. I don't always have explanation for these numbers, except that they are part of the formula.
I don't really know what to do with these. Magic numbers scream at me, but I can't invent names or use confusing names that might not be accurate.

That's fair! I was hoping the book that provided the equation also provided an explanation of what these numbers were. Or at least gave their unit!

rhannequin self-assigned this Feb 18, 2024

rhannequin force-pushed the angles-as-a-single-class branch from f6748c1 to 12e2fae Compare February 18, 2024 19:40

JoelQ approved these changes Feb 19, 2024

View reviewed changes

rhannequin added 2 commits February 19, 2024 21:35

Introduce Angle.zero

9111314

Add constants for Angle and increase precision

d308371

rhannequin merged commit 5449e79 into main Feb 19, 2024
5 checks passed

rhannequin deleted the angles-as-a-single-class branch February 19, 2024 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Angle as a single class #13

Refactor Angle as a single class #13

rhannequin commented Feb 18, 2024 •

edited

Loading

JoelQ left a comment

JoelQ Feb 19, 2024

JoelQ Feb 19, 2024

JoelQ Feb 19, 2024

JoelQ Feb 19, 2024

JoelQ Feb 19, 2024

rhannequin Feb 19, 2024

JoelQ Feb 19, 2024

JoelQ Feb 19, 2024

JoelQ Feb 19, 2024

JoelQ Feb 19, 2024

JoelQ Feb 19, 2024

JoelQ Feb 19, 2024

JoelQ Feb 19, 2024

rhannequin Feb 20, 2024

JoelQ Feb 21, 2024

		@sun_longitude.radians - @coordinates.longitude.radians
		) / Math.cos(@coordinates.latitude.radians) / 3600

		Math.sin(@sun_longitude.radians - @coordinates.longitude.radians) *
		Math.sin(@coordinates.latitude.radians) / 3600

		-17.2 * Math.sin(moon_ascending_node_longitude.radians) -
		1.3 * Math.sin(2 * sun_mean_longitude.radians)

Refactor Angle as a single class #13

Refactor Angle as a single class #13

Conversation

rhannequin commented Feb 18, 2024 • edited Loading

JoelQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhannequin commented Feb 18, 2024 •

edited

Loading