2

I have an algorithm I am trying to implement that has steps 1 to 5. There are several different ways I could implement each step. Each calculation step is essentially just an astronomy calculation, and I could either depend on any one of the astronomy libraries or implement my own astronomy calculation.

My current design seems to be too heavily dependent on one library throughout all of the astronomy calculations, and I am concerned about what I will have to do when I have to change how I calculate a step. I would rather not have so much hardcoding, and I would like my development to be a bit easier in the future.

Is my dependency route overkill? What would be a good way to design this software? I just need an astronomy module with a collection of astronomy functions, but I feel like the Algorithm should be insulated from how the astronomy module changes.

Current Design

algorithm.py

import astronomy

class Algorithm:
   def __init__(self) -> None:
      pass

   def calculate(self, a: float, b: float, c: dt.datetime) -> dict[str, tuple]:
      value1 = astronomy.calculation1(a, b, c)
      value2 = astronomy.calculation2(a, b, c)
      value3 = astronomy.calculation3(value1, value2)
      value4 = astronomy.calculation4(value3)
      value5 = astronomy.calculation5(value1)

      return { ... }

astronomy.py

import skyfield

# import lots of other libraries

def calculation1(self, a, b, c):
   """
   lots of skyfield specific code
   """

def calculation2(self, a, b, c):
   """
   lots of skyfield specific code
   """

def calculation3(self, x, y):
   """
   lots of skyfield specific code
   """

def calculation4(self, x):
   """
   lots of skyfield specific code
   """

def calculation5(self, x):
   """
   lots of skyfield specific code
   """

New Design with Dependency Injection

algorithm.py

import AstronomyCalculator

class Algorithm:
   def __init__(self, astronomy_calculator) -> None:
      self.ac = astronomy_calculator

   def calculate(self, a: float, b: float, c: dt.datetime) -> dict[str, tuple]:
      value1 = self.ac.calculation1(a, b, c)
      value2 = self.ac.calculation2(a, b, c)
      value3 = self.ac.calculation3(value1, value2)
      value4 = self.ac.calculation4(value3)
      value5 = self.ac.calculation5(value1)

      return { ... }

astronomycalculator.py

import ABC

class AstronomyCalculator(ABC):
   def __init__(self):
      pass

   def calculation1(a, b, c):
      pass

   def calculation2(a, b, c):
      pass

   def calculation3(value1, value2):
      pass

   def calculation4(value3):
      pass

   def calculation5(value1):
      pass

skyfield.py

import skyfield

class Skyfield(AstronomyCalculator):
   def __init__(self):
      pass

   def calculation1():
      """
      lots of skyfield specific code
      """

astropy.py

import astropy

class Astropy(AstronomyCalculator):
   def __init__(self):
      pass

   def calculation1():
      """
      lots of astropy specific code
      """

custommath.py

import numpy as np
import pandas as pd

class CustomMath(AstronomyCalculator):
   def __init__(self):
      pass

   def calculation1(a, b, c):
      """
      handwritten calculations
      """

main.py

def main():
   # Then I could change which astronomy calculator I use here
   astronomy_utils = Skyfield()
   algorithm = Algorithm(astronomy_utils)

   results = algorithm.calculate(a,b,c)

EDIT:

To add further context about what the algorithm is doing, here is the calculate() function:

class Algorithm:
   def __init__(self) -> None:
      pass

   def calculate(self, latitude: float, longitude: float, local_time: dt.datetime) -> dict[str, tuple]:
      for body in constants.CELESTIAL_INTERESTS:
         upper_culmination_time = astronomy.meridian_transit_time(latitude, longitude, local_time, body)
         rise_time = astronomy.rise_time(latitude, longitude, local_time, body)
         set_time = astronomy.set_time(latitude, longitude, local_time, body)
         
         ascending_positions_horizontal_reference_frame = astronomy.sample_ephemeride(body, latitude, longitude, rise_time, upper_culmination_time, samples = 100)
         descending_positions_horizontal_reference_frame = astronomy.sample_ephemeride(body, latitude, longitude, upper_culmination_time, set_time, samples = 100)

         ascending_positions_geodetic_reference_frame = astronomy.subpoints_of_positions(ascending_positions_horizontal_reference_frame)
         descending_positions_geodetic_reference_frame = astronomy.subpoints_of_positions(descending_positions_horizontal_reference_frame)

      return { ... }

The functions are nearly independent of each other. They are simply a collection of astronomy functions.

3
  • What is the (realistic) likelihood that you will be hot-swapping libraries that perform exactly the same task, and is it something that'll happen often or even any time soon? The majority of the effort in replacing dependencies is nearly always about regression testing rather than development work. A more valuable question might be whether this makes your life significantly easier to write your tests; but the only way you can really know that is to start out by actually writing those tests (Where your time is likely far better spent anyway). Commented Feb 5 at 12:54
  • 2
    YAGNI: you ain't gonna need it.
    – pjc50
    Commented Feb 5 at 17:02
  • Not a direct answer here but you can always import a function name e.g., import calculation1 from astronomy. Then swapping that out with a new equivalent function can be done simply by changing it to import calculation1 from foobar or import calculationx from foobar as calculation1
    – JimmyJames
    Commented Feb 5 at 17:48

2 Answers 2

5

Your new approach isn't inherently wrong. However, there are a few things I would like to question here:

  • Currently, the Algorithm.calculate function in your example contains almost no generic, astro-library independent functionality (except for the chaining of 5 methods calculate1 to calculate5). This makes me wonder why you don't swap calculate out "as a whole", that would probably be sufficient.

  • You wrote

    I am concerned about the future in the event I have to change how I calculate a step

    which gives me a warning sign since it seems you are investing effort "just in case", without a real need. I would defer such a decision until I really have to integrate a second library or a different calculation into the code, because otherwise I usually get the separation between generic and non-generic parts wrong, or at least sub-optimal. For example, it may turn out you don't want to swap out all calculations all-at-once, but only one or two of them, in which case your "new" approach is not suitable. Note also that your current design already provides a logical separation between a skyfield-independend Algorithm class and a skyfield-dependend astronomy.py module. Just keep it that way, then it should not be too hard to switch to a DI-based design at a later point in time.

One might check if the real Algorithm class contains more than just glue code, and if it is worth to unit-test the contained code with an injected AstronomyCalculatorMock, to isolate the tests from the skyfield module. Currently, however, it does not look like this would give a huge benefit, not just because the code inside Algorithm is trivial, but also because writing tests with the real skyfield module may not be a problem. In case such tests are fast enough and stable, there is usually no need to mock anything here.

1
  • Thanks so much for the feedback. You're right, I'll maintain the current design and not stress too much about the future. The algorithm class is actually that trivial, there's not much going on in it. I added an edit to the post to explain more about what it's doing for further context
    – Hunter
    Commented Feb 5 at 20:46
3

If you just call your functions "Algorithm" and "Calculate" then it's hard to make a judgement about the structure of your code.

Should Calculate1 through 5 be separate functions? Only if they are reusable units of code that make sense, ie calc1 is calcRightAscension or something.

Should Calculate1 through 5 be part of a single class? Only if they are related to each other and the class contains data that's reused by the different functions. ie star.CalcSpectrum(), star.CalcBrightness(). Otherwise they can be static methods in their own namespace or multiple classes etc

Should astronomy with all the calculate functions be abstracted so that various versions can be used via dependency injection. Only if you are expecting to have various implementation. ie starRedGiant, starBlueDwarf or its useful for unit testing.

You already have encapsulated the functions that use the skyfield library in their own class, so if you ever did need to replace the library it would be easy to write a new astronomy class and use it instead. Possible library replacement won't cause you huge problems so it shouldn't a be a be driver of structure.

1
  • Thanks so much for the feedback! Yeah the functions are pretty much all independent from one another. I added an edit to the original post adding more context about what the algorithm is doing. I think I will stick with the current design considering all of the feedback here
    – Hunter
    Commented Feb 5 at 20:54

Not the answer you're looking for? Browse other questions tagged or ask your own question.