Skip to content

Rewrote parts of the codebase to support python function arguments #107

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.

Sign up for GitHub

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

Open
eiseleb47 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Rewrote parts of the codebase to support python function arguments #107

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0b3a0dc
Switched from cli input to python function
eiseleb47 Jun 13, 2025
b253d9d
Added backwards compatability for CLI arguments
eiseleb47 Jun 15, 2025
dcede5b
Renamed irdb_path and removed dublicated code
eiseleb47 Jun 17, 2025
63d08a8
Added back catglist
eiseleb47 Jun 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions Simulations/python/downloadPackages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
#!/usr/bin/env python

import scopesim as sim
from pathlib import Path

sim.download_packages("METIS",release="github:dev_master")
sim.download_packages("Armazones", release="2023-07-11")
sim.download_packages("ELT", release="2024-02-29")
pkgs = ["METIS", "Armazones", "ELT"]
Copy link
Contributor

@hugobuddel hugobuddel Jun 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not the same, because the release is not mentioned anymore

Copy link
Contributor Author

@eiseleb47 eiseleb47 Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed them because the dev_master release doesn't work with the current ScopeSim version. Stable works with the current one, but I can of course change them to specific versions that work, although I don't know which release that would be.


for pkg in pkgs:
sim.download_packages(pkg,release="stable", save_dir=str(Path.home()) + '/.inst_pkgs' )
Copy link
Contributor

@hugobuddel hugobuddel Jun 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to just leave the directory the default.

Copy link
Contributor Author

@eiseleb47 eiseleb47 Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the current working directory? As that is the default in ScopeSim.

Copy link
Contributor

@hugobuddel hugobuddel Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that all our projects use the same default setting, unless there is a strong reason to deviate from that, which I don't think applies here. (Not that the current default is particularly good, but that is another thing.)

13 changes: 8 additions & 5 deletions Simulations/python/raw_script.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@
import astropy.units as u

from simulationDefinitions import *
from sources import *
#sim.rc.__config__["!SIM.file.local_packages_path"] = DEFAULT_IRDB_LOCATION
import sources

logger = get_logger(__file__)

Expand All @@ -34,7 +33,7 @@
# Current Solution: use the mode field in the YAML file directly.
# may want to error trap for invalid combinations in the future

def simulate(fname, mode, kwargs, wcu, source=None, small=False):
def simulate(scopesim_path, fname, mode, kwargs, wcu, source=None, small=False ):

"""Run main function for this script."""
logger.info("*****************************************")
Expand All @@ -45,6 +44,10 @@ def simulate(fname, mode, kwargs, wcu, source=None, small=False):
logger.debug("output filename: %s", fname)
else:
logger.warning("Output filename not set, result will not be saved.")

if scopesim_path is not None:
sources.setup_scopesim_sources(scopesim_path)
SOURCEDICT = sources.get_SOURCEDICT()

if isinstance(source, Mapping):
src_name = source["name"]
Expand Down Expand Up @@ -84,9 +87,9 @@ def simulate(fname, mode, kwargs, wcu, source=None, small=False):

#set up the simulation
if("wavelen" in kwargs["OBS"].keys()):
cmd = sim.UserCommands(use_instrument="metis", set_modes=[mode],properties={"!OBS.wavelen": kwargs["OBS"]['wavelen']})
cmd = sim.UserCommands(use_instrument="METIS", set_modes=[mode],properties={"!OBS.wavelen": kwargs["OBS"]['wavelen']})
else:
cmd = sim.UserCommands(use_instrument="metis", set_modes=[mode])
cmd = sim.UserCommands(use_instrument="METIS", set_modes=[mode])

#copy over the OBS settings directly, then set up the optical train

Expand Down
49 changes: 34 additions & 15 deletions Simulations/python/runRecipes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@
from datetime import datetime
import numpy as np
from astropy.io import fits
import astropy
import copy
from multiprocessing import Pool,Process,Manager
from multiprocessing import Pool

class runRecipes():

Expand All @@ -29,7 +27,8 @@ def __init__(self):
self.tDelt = TimeDelta(0, format='sec')
self.allFileNames = []
self.allmjd = []

self.params = {}

def parseCommandLine(self,args):

"""
Expand Down Expand Up @@ -73,6 +72,9 @@ def parseCommandLine(self,args):
parser.add_argument('-n', '--nCores', type=int,
default = 1,
help='number of cores for parallel processing')
parser.add_argument('-p', '--scopesim_path', type=str,
default = str(Path.home()) + '/.inst_pkgs',
Copy link
Contributor

@hugobuddel hugobuddel Jun 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just keep the default the same default as ScopeSim uses. It is confusing to use a different default in different projects.

Also, the parameter should perhaps not be called scopesim_path, since it is not that, it is the instrument packages path or the IRDB path. So maybe pkgs_path? or irdb_path?

Copy link
Contributor Author

@eiseleb47 eiseleb47 Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I'll rename the argument and see to it that I set the same path as in ScopeSim. This was mostly an oversigth by me when testing that the directory was found which I forgot to change later.

help='Path to the IRDB instrument packages')


args = parser.parse_args()
Expand Down Expand Up @@ -120,6 +122,9 @@ def parseCommandLine(self,args):
else:
params['nCores'] = 1

if args.scopesim_path:
params['scopesim_path'] = args.scopesim_path

print(f"Starting Simulations")
print(f" input YAML = {params['inputYAML']}, output directory = {params['outputDir']}")
if(params['startMJD'] is not None):
Expand All @@ -133,6 +138,28 @@ def parseCommandLine(self,args):
print(f" Generate External Calibs {params['doCalib']}")

self.params = params

def setParms(self, **params):
"""
Set parameters directly from keyword arguments or a dictionary.
"""
self.params = params

print(f"Starting Simulations")
print(f" input YAML = {params['inputYAML']}, output directory = {params['outputDir']}")
if(params['sequence'] == "1"):
params['startMJD'] = None
params['sequence'] = True
else:
params['startMJD'] = params['sequence']
params['sequence'] = True
if(params['sequence']):
print(f" Observation sequence will start from first date in YAML file")
else:
print(f" Observation dates will be taken from YAML file if given")
print(f" Automatically generated darks and flats {params['doCalib']}")
print(f" Small output option {params['small']}")
print(f" Generate External Calibs {params['doCalib']}")

def loadYAML(self):

Expand All @@ -157,16 +184,8 @@ def _filter_yaml(self,allrcps):

"""filter a dictionary of recipes"""

if self.params['catglist'] is None:

dorcps = allrcps
else:
dorcps = {}
for catg in self.params['catglist']:
if catg in allrcps.keys():
dorcps[catg] = allrcps[catg]
else:
raise ValueError(f"ERROR: {catg} is not a supported product category")
dorcps = allrcps

Copy link
Contributor

@hugobuddel hugobuddel Jun 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the catglist selection?

Copy link
Contributor Author

@eiseleb47 eiseleb47 Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it originally when I tried to get the simulations working with function arguments. I forgot to add them back later on, but will do so now.

return dorcps

def validateYAML(self):
Expand Down Expand Up @@ -739,7 +758,7 @@ def _run(self,dorcps):
recipe["wcu"] = None
#simulate(fname, mode, kwargs,recipe["wcu"], source=recipe["source"], small=self.params['small'])

allArgs.append((fname,mode,kwargs,recipe["wcu"],recipe["source"],self.params["small"]))
allArgs.append((self.params["scopesim_path"],fname,mode,kwargs,recipe["wcu"],recipe["source"],self.params["small"]))

if(not self.params['testRun']):
nCores = self.params['nCores']
Expand Down
93 changes: 75 additions & 18 deletions Simulations/python/run_recipes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,42 +8,71 @@
import runRecipes as rr
import sys
import makeCalibPrototypes


def runRecipes(argv):

from pathlib import Path

def runRecipes(inputYAML=Path.joinpath(Path(__file__).parent.parent,
"YAML/allRecipes.yaml"),
Copy link
Contributor

@hugobuddel hugobuddel Jun 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need the joinpath, you can do this: Path(__file__).parent.parent / "YAML" / "allRecipes.yaml",nice aye?

outputDir="output/",
small=False,
doStatic=False,
doCalib=0,
sequence="1",
testRun=False,
calibFile=None,
nCores=8,
scopesim_path=Path.home() / ".sigmas_pkg",
Copy link
Contributor

@hugobuddel hugobuddel Jun 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎶 Sigma sigma boy sigma boy 🎶

**kwargs):
"""
Run a set of recipes with explicit arguments instead of command line.
Arguments:
inputYAML: Path to the input YAML file.
doCalib: Integer flag for calibration.
calibFile: Path to calibration file.
testRun: Boolean flag for test run.
doStatic: Boolean flag for static calibration.
outputDir: Output directory.
kwargs: Any additional parameters.
"""
simulationSet = rr.runRecipes()

#simulationSet.setParms(inputYAML = None) \TODO - set parameters directly rather than commandline

# get the command line arguments

simulationSet.parseCommandLine(argv[1:])
# Set parameters directly
simulationSet.setParms(
scopesim_path=scopesim_path,
inputYAML=inputYAML,
outputDir=outputDir,
small=small,
doStatic=doStatic,
doCalib=doCalib,
sequence=sequence,
testRun=testRun,
calibFile=calibFile,
nCores=nCores,
**kwargs
)

# read in the YAML
simulationSet.loadYAML()

# validate the YAML

goodInput = simulationSet.validateYAML()

# exit if the YAML entries are not valid
if (not goodInput):
exit
if not goodInput:
return

# if requested, get the list of flats and darks
if(simulationSet.params['doCalib'] > 0):
if simulationSet.params['doCalib'] > 0:
simulationSet.calculateCalibs()

# run the simulations
simulationSet.runSimulations()

# run the calibrations if requested
if(simulationSet.params['doCalib'] > 0):
if simulationSet.params['doCalib'] > 0:
simulationSet.runCalibrations()

# if requested, dump the calibration dictionary to a YAML file in the same format as the input YAML
if(simulationSet.params['calibFile'] is not None):
if simulationSet.params['calibFile'] is not None:
simulationSet.dumpCalibsToFile()

simulationSet.allFileNames.sort()
Expand All @@ -52,12 +81,40 @@ def runRecipes(argv):


# if simulations were done, update the headers
if(not simulationSet.params['testRun']):
if not simulationSet.params['testRun']:
simulationSet.updateHeaders()

if(simulationSet.params['doStatic']):
if simulationSet.params['doStatic']:
makeCalibPrototypes.generateStaticCalibs(simulationSet.params['outputDir'])

if __name__ == "__main__":
simulationSet = rr.runRecipes()
simulationSet.parseCommandLine(sys.argv[1:])
Copy link
Contributor

@hugobuddel hugobuddel Jun 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, this doesn't seem to make sense: all the code is now duplicated and not in a separate function anymore. Neither of those is good; my suggestion to make a PR was exactly to prevent code duplication.

What I expected was that you would refactor the runRecipes() functions into two parts, so the code would look something like:

def runRecipes_with_pars(
    inputYaml=...,
    outputDir=...,
    ...
):

def runRecipes_with_argv(argv):
    params = parseCommandLine(argv[1:])
    runRecipes_with_pars(**params)

if __name__ == "__main__":
    runRecipes_with_argv(sys.argv)

Or something equivalent. (These names are snakecamelcase...)

Copy link
Contributor Author

@eiseleb47 eiseleb47 Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was kind of tried when I made that decision with the code duplication. I'll rewrite the file to be in line with your suggestion 👍


simulationSet.loadYAML()

runRecipes(sys.argv)
goodInput = simulationSet.validateYAML()

if (not goodInput):
exit

if(simulationSet.params['doCalib'] > 0):
simulationSet.calculateCalibs()

simulationSet.runSimulations()

if(simulationSet.params['doCalib'] > 0):
simulationSet.runCalibrations()

if(simulationSet.params['calibFile'] is not None):
simulationSet.dumpCalibsToFile()

simulationSet.allFileNames.sort()
for elem in simulationSet.allFileNames:
print(elem)

if(not simulationSet.params['testRun']):
simulationSet.updateHeaders()

if(simulationSet.params['doStatic']):
makeCalibPrototypes.generateStaticCalibs(simulationSet.params['outputDir'])
Loading
Loading