Rewrote parts of the codebase to support python function arguments #107
Conversation
hugobuddel
left a comment
hugobuddel
left a comment
There was a problem hiding this comment.
Yes, I think it would be better to make it easier to be able to import these files and run just the functions.
However, I was expecting you would just change the parameter parsing. Now there is too much code duplication in the __main__ block; that should be abstracted away in a function.
There are some smaller things that now seem broken, in particular downloading the specific package versions, and also providing a catg list it seems.
In the end I'll let @JenniferKarr decide what is or is not to be accepted.
| 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"] |
There was a problem hiding this comment.
These are not the same, because the release is not mentioned anymore
There was a problem hiding this comment.
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.
| pkgs = ["METIS", "Armazones", "ELT"] | ||
|
|
||
| for pkg in pkgs: | ||
| sim.download_packages(pkg,release="stable", save_dir=str(Path.home()) + '/.inst_pkgs' ) |
There was a problem hiding this comment.
I think it is better to just leave the directory the default.
There was a problem hiding this comment.
Do you mean the current working directory? As that is the default in ScopeSim.
There was a problem hiding this comment.
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.)
Simulations/python/runRecipes.py
Outdated
| default = 1, | ||
| help='number of cores for parallel processing') | ||
| parser.add_argument('-p', '--scopesim_path', type=str, | ||
| default = str(Path.home()) + '/.inst_pkgs', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Simulations/python/run_recipes.py
Outdated
| testRun=False, | ||
| calibFile=None, | ||
| nCores=8, | ||
| scopesim_path=Path.home() / ".sigmas_pkg", |
There was a problem hiding this comment.
🎶 Sigma sigma boy sigma boy 🎶
Simulations/python/runRecipes.py
Outdated
| dorcps = allrcps | ||
|
|
There was a problem hiding this comment.
Why remove the catglist selection?
There was a problem hiding this comment.
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.
Simulations/python/run_recipes.py
Outdated
| def runRecipes(inputYAML=Path.joinpath(Path(__file__).parent.parent, | ||
| "YAML/allRecipes.yaml"), |
There was a problem hiding this comment.
You don't need the joinpath, you can do this: Path(__file__).parent.parent / "YAML" / "allRecipes.yaml",nice aye?
Simulations/python/run_recipes.py
Outdated
| if __name__ == "__main__": | ||
| simulationSet = rr.runRecipes() | ||
| simulationSet.parseCommandLine(sys.argv[1:]) |
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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 👍
| def get_skyStarT(): | ||
| return _scopesim_state["skyStarT"] | ||
|
|
||
| def get_SOURCEDICT(): |
There was a problem hiding this comment.
Yeah... I understand. This is rather ugly, but I can't complain because I have done the exact same thing before...
2 participants
I rewrote parts of
to allow the topmost function in run_recipes to be used in a pythonic way. The command line usage via runESO.sh is still supported.
Additionally it is now possible to set a custom location for the IRDB packages instead of defaulting to the current working directory.