1044 get rid of all global variables and move to a config data class #1077

Little-Ryugu · 2024-12-05T12:48:31Z

Fixes #1044
Fixes #1015
Created a new data class called auxiliaryConfigs. This data class contains the default filenames and URLs that Sorcha needs for ephemeris generation. These default filenames and URLs can be overwritten in the [AUXILIARY] section of the config file when running Sorcha. This data class replaces the global variables for these files.

Review Checklist for Source Code Changes

Does pip install still work?
Have you written a unit test for any new functions?
Do all the units tests run successfully?
Does Sorcha run successfully on a test set of input files/databases?
Have you used black on the files you have updated to confirm python programming style guide enforcement?

This dataclass takes in filename and urls from the AUXILILARY section of the config file. If AUXILIARY section does not exist, default filenames and URLs are instead used.

changed function make_retriever use the config dataclass auxiliary. Went through the ephemeris files and updated this funtion. Completely removed all the global variables in similation_data_files.py.

…e-to-a-config-data-class

removed global variables being imported

mschwamb · 2024-12-05T13:51:16Z

The review needs to check sorcha bootstrap works so does sorcha run with the aux files already downloaded, and also sorcha run with the files needing to be downloaded. @Little-Ryugu has tested this locally

mschwamb · 2024-12-05T14:13:38Z

@bernardinelli would you be able to test and see how we would test this on an order version of the files inputting in the config file?

Little-Ryugu · 2024-12-05T14:27:39Z

An example of the config file format for initialising a new file.

[AUXILIARY]
#Using file de432.bsp instead of the default file for de440s.
#filename
de440s = de432.bsp
#url of file
de440s_url = https://naif.jpl.nasa.gov/pub/naif/generic_kernels/spk/planets/de432.bsp

bernardinelli · 2024-12-11T16:00:27Z

src/sorcha/utilities/sorchaConfigs.py

@@ -820,6 +1015,9 @@ class sorchaConfigs:
    expert: expertConfigs = None
    """expertConfigs dataclass which stores the keywords from the EXPERT section of the config file."""

+    auxiliary: auxiliaryConfigs = None
+    """auxiliaryConfigs dataclass which stores the keywords from the AUXILILARY section of the config file."""


Typo AUXILILARY :D

mschwamb · 2024-12-10T10:59:15Z

src/sorcha/ephemeris/simulation_parsing.py

@@ -129,35 +125,42 @@ def parse_orbit_row(row, epochJD_TDB, ephem, sun_dict, gm_sun, gm_total):
    return tuple(np.concatenate([equatorial_coords, equatorial_velocities]))


+default = sorchaConfigs()


these are now global variables because the are outside of any function. Can you have oc_file=None and if it's None then you create default? within class Obsevatory

mschwamb · 2024-12-10T10:59:26Z

src/sorcha/ephemeris/simulation_parsing.py

@@ -129,35 +125,42 @@ def parse_orbit_row(row, epochJD_TDB, ephem, sun_dict, gm_sun, gm_total):
    return tuple(np.concatenate([equatorial_coords, equatorial_velocities]))


+default = sorchaConfigs()
+default.auxiliary = auxiliaryConfigs()  # using the default observatory_codes in the auxiliaryConfigs class


these are now global variables because the are outside of any function. Can you have oc_file=None and if it's None then you create default? within class Obsevatory

Do we need all of default or can you just make an
auxConfig = auxiliaryConfigs() and send that around. If we're not using any part of sconfigs other than auxConfig bits i'd just send that through

mschwamb · 2024-12-10T11:03:10Z

src/sorcha/ephemeris/simulation_parsing.py

 class Observatory:
    """
    Class containing various utility tools related to the calculation of the observatory position
    """

-    def __init__(self, args, oc_file=OBSERVATORY_CODES):
+    def __init__(self, args, sconfigs, oc_file=default.auxiliary.observatory_codes):


I think if sconfigs is None than you set oc_file- do you really need to set oc_file here with a global?

mschwamb · 2024-12-10T11:28:35Z

src/sorcha/ephemeris/simulation_setup.py

-    retriever = make_retriever(args.ar_data_file_path)
-    planets_file_path = retriever.fetch(JPL_PLANETS)
-    small_bodies_file_path = retriever.fetch(JPL_SMALL_BODIES)
+    retriever = make_retriever(sconfigs, args.ar_data_file_path)


I'd consider only passing it the ephemeris config bits since we use this in bootstrap utility where we don't have a config file class loaded since it's doesn't read one in @bernardinelli what do you think?

Yeah, that makes sense. One way to do this would be to have an independent class or dictionary or something has only the ephemeris stuff, so that it can also be created independently in bootstrap. Or if sconfigs doesn't need to have everything set up, it'd accomplish the same thing (with no need to change the code)

Apparently I commented and did not finish the review so no one could see me comments. Thanks @Little-Ryugu for pointing that out. - there is a smaller ephemeris auxiliary file class instance that is made within the sconfigs class so we should just be able to pass the ephemeris auxiliary file class here

Changed make_retriever to only take aux config. Removed global variable in for class Observatory.

changed functions create_assist_ephemeris and furnish_spiceypy to only get the aux data class. Also added comment in bootstrap.py on how to download new files

mschwamb

This works for me as intended

Little-Ryugu added 11 commits November 28, 2024 12:57

Created new config dataclass auxililaryConfigs

f75e72d

This dataclass takes in filename and urls from the AUXILILARY section of the config file. If AUXILIARY section does not exist, default filenames and URLs are instead used.

cleaned up the if statements in auxililaryConfigs

22e1663

changing the auxililary dataclass and making first unit test

6768188

made changes to auxiliaryConfigs and to it's unit test

ab2b214

Created unit tests for auxiliaryConfigs

dba96ff

changed make_retriever function to work with dataclasses

dd4dde2

Update sorchaConfigs.py

82c92d9

changed function make_retriever and updated code

3098b60

changed function make_retriever use the config dataclass auxiliary. Went through the ephemeris files and updated this funtion. Completely removed all the global variables in similation_data_files.py.

added docs for some functions

2345ca5

black reformatting

c857857

Merge branch 'main' into 1044-get-rid-of-all-global-variables-and-mov…

552e768

…e-to-a-config-data-class

Little-Ryugu linked an issue Dec 5, 2024 that may be closed by this pull request

get rid of all global variables and move to a config data class #1044

Closed

Little-Ryugu requested review from mschwamb and astronomerritt December 5, 2024 12:53

mschwamb requested a review from bernardinelli December 5, 2024 13:38

Update retrieve_ephemeris_data_files.py

d4cf482

removed global variables being imported

mschwamb removed the request for review from astronomerritt December 10, 2024 11:32

bernardinelli reviewed Dec 11, 2024

View reviewed changes

fixed typo

7e006bc

mschwamb requested changes Dec 12, 2024

View reviewed changes

Little-Ryugu added 3 commits December 13, 2024 14:21

Changed make_retriever to only take aux config attributes

331b122

Changed make_retriever to only take aux config. Removed global variable in for class Observatory.

Changed Observatory class to only get aux data class

c1d880d

Changed more functions to only get aux data class

914398b

changed functions create_assist_ephemeris and furnish_spiceypy to only get the aux data class. Also added comment in bootstrap.py on how to download new files

mschwamb self-requested a review December 18, 2024 16:09

mschwamb approved these changes Dec 18, 2024

View reviewed changes

Little-Ryugu merged commit 6ff172b into main Dec 18, 2024
7 checks passed

mschwamb mentioned this pull request Dec 18, 2024

Add a configuration option to use integrate over integrate_or_interpolate #1034

Open

5 tasks

mschwamb deleted the 1044-get-rid-of-all-global-variables-and-move-to-a-config-data-class branch January 18, 2025 10:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1044 get rid of all global variables and move to a config data class #1077

1044 get rid of all global variables and move to a config data class #1077

Little-Ryugu commented Dec 5, 2024 •

edited by mschwamb

Loading

mschwamb commented Dec 5, 2024

mschwamb commented Dec 5, 2024

Little-Ryugu commented Dec 5, 2024

bernardinelli Dec 11, 2024

mschwamb Dec 10, 2024

mschwamb Dec 10, 2024

mschwamb Dec 10, 2024

mschwamb Dec 10, 2024

mschwamb Dec 10, 2024

bernardinelli Dec 12, 2024

mschwamb Dec 12, 2024

mschwamb left a comment

		@@ -129,35 +125,42 @@ def parse_orbit_row(row, epochJD_TDB, ephem, sun_dict, gm_sun, gm_total):
		return tuple(np.concatenate([equatorial_coords, equatorial_velocities]))


		default = sorchaConfigs()

1044 get rid of all global variables and move to a config data class #1077

1044 get rid of all global variables and move to a config data class #1077

Conversation

Little-Ryugu commented Dec 5, 2024 • edited by mschwamb Loading

Review Checklist for Source Code Changes

mschwamb commented Dec 5, 2024

mschwamb commented Dec 5, 2024

Little-Ryugu commented Dec 5, 2024

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

mschwamb left a comment

Choose a reason for hiding this comment

Little-Ryugu commented Dec 5, 2024 •

edited by mschwamb

Loading