-
Notifications
You must be signed in to change notification settings - Fork 28
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.
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
Set min_radius_pix
for GalSim unit tests
#538
Set min_radius_pix
for GalSim unit tests
#538
Conversation
It's currently necessary to override this parameter to make galsim unit tests pass (see jeff-regier#534). This is the simplest way to code it but it's ugly in my opinion. We should discuss alternatives in the PR thread or an issue thread.
I should mention for reference that the |
Let's bring up some of these issues with Keno tomorrow. I bet he has away do it that won't leave you missing OOP. |
OK. I added some abstract types in my sample code where they ought to be used. For what it's worth I don't think the above solution is necessarily bad, I can just see how some might find it overly complex, and I'm open to simpler alternatives. It's a matter of taste. |
As discucsed in PR jeff-regier#538, we have this problem of passing arguments deep down the call stack of Celeste inference. This manifests itself as lots of chains of keyword arguments and various not-so-pretty hacks (like custom `infer_source_callback` wrappers) to set options. The discussion in jeff-regier#538 outlines a broad direction for not only solving this problem, but also the related problem of different branches of code for different options at each stage of inference. The two current examples are single vs. joint inference and MOG vs. FFT PSF inference, but there will likely always be such branches. I still think that's a good direction to go, but even a minimal implementation is too invasive to be safe/worthwhile right now. Instead, this commit takes what I view as the simplest possible step in the right direction. We add a single `Config` object which is instantiated at top-level and passed as the first argument through all major function calls. Over time this object ought to absorb more parameters. Once there are distinct groups of parameters, the object can be split into multiple objects, and functions can receive only the sub-Config-object they require. Dynamic dispatch of different types of sub-Config-objects can follow from that. (If this doesn't make sense, read the example in jeff-regier#538.) The change looks a little silly right now, since it's this config object with one parameter, but it's an incremental path. There are a lot of legacy wrappers included which instantiate the default Config object, so I don't have to go change every top-level caller (in which case I'd likely break something because I'm not familiar with much of the code), but over time callers ought to be changed to explicitly pass a Config object, and the legacy wrappers can go away.
As discucsed in PR jeff-regier#538, we have this problem of passing arguments deep down the call stack of Celeste inference. This manifests itself as lots of chains of keyword arguments and various not-so-pretty hacks (like custom `infer_source_callback` wrappers) to set options. The discussion in jeff-regier#538 outlines a broad direction for not only solving this problem, but also the related problem of different branches of code for different options at each stage of inference. The two current examples are single vs. joint inference and MOG vs. FFT PSF inference, but there will likely always be such branches. I still think that's a good direction to go, but even a minimal implementation is too invasive to be safe/worthwhile right now. Instead, this commit takes what I view as the simplest possible step in the right direction. We add a single `Config` object which is instantiated at top-level and passed as the first argument through all major function calls. Over time this object ought to absorb more parameters. Once there are distinct groups of parameters, the object can be split into multiple objects, and functions can receive only the sub-Config-object they require. Dynamic dispatch of different types of sub-Config-objects can follow from that. (If this doesn't make sense, read the example in jeff-regier#538.) The change looks a little silly right now, since it's this config object with one parameter, but it's an incremental path. There are a lot of legacy wrappers included which instantiate the default Config object, so I don't have to go change every top-level caller (in which case I'd likely break something because I'm not familiar with much of the code), but over time callers ought to be changed to explicitly pass a Config object, and the legacy wrappers can go away.
As discucsed in PR jeff-regier#538, we have this problem of passing arguments deep down the call stack of Celeste inference. This manifests itself as lots of chains of keyword arguments and various not-so-pretty hacks (like custom `infer_source_callback` wrappers) to set options. The discussion in jeff-regier#538 outlines a broad direction for not only solving this problem, but also the related problem of different branches of code for different options at each stage of inference. The two current examples are single vs. joint inference and MOG vs. FFT PSF inference, but there will likely always be such branches. I still think that's a good direction to go, but even a minimal implementation is too invasive to be safe/worthwhile right now. Instead, this commit takes what I view as the simplest possible step in the right direction. We add a single `Config` object which is instantiated at top-level and passed as the first argument through all major function calls. Over time this object ought to absorb more parameters. Once there are distinct groups of parameters, the object can be split into multiple objects, and functions can receive only the sub-Config-object they require. Dynamic dispatch of different types of sub-Config-objects can follow from that. (If this doesn't make sense, read the example in jeff-regier#538.) The change looks a little silly right now, since it's this config object with one parameter, but it's an incremental path. There are a lot of legacy wrappers included which instantiate the default Config object, so I don't have to go change every top-level caller (in which case I'd likely break something because I'm not familiar with much of the code), but over time callers ought to be changed to explicitly pass a Config object, and the legacy wrappers can go away.
) * add a logger that includes exception traceback, and wrap `Thread.@threads` loops with it * `Log.exception()` is like `Log.error()`, but includes exception information. Inspired by Python's `logging.exception()` and such. * Since exceptions inside threads are silently swallowed right now (JuliaLang/julia#17532), always wrap the body of a `Thread.@threads` call with a try-catch that manually logs the exception. * fix bug where i forgot to add the initial position offset * add Config object to faciliate passing params deep into the call chain As discucsed in PR #538, we have this problem of passing arguments deep down the call stack of Celeste inference. This manifests itself as lots of chains of keyword arguments and various not-so-pretty hacks (like custom `infer_source_callback` wrappers) to set options. The discussion in #538 outlines a broad direction for not only solving this problem, but also the related problem of different branches of code for different options at each stage of inference. The two current examples are single vs. joint inference and MOG vs. FFT PSF inference, but there will likely always be such branches. I still think that's a good direction to go, but even a minimal implementation is too invasive to be safe/worthwhile right now. Instead, this commit takes what I view as the simplest possible step in the right direction. We add a single `Config` object which is instantiated at top-level and passed as the first argument through all major function calls. Over time this object ought to absorb more parameters. Once there are distinct groups of parameters, the object can be split into multiple objects, and functions can receive only the sub-Config-object they require. Dynamic dispatch of different types of sub-Config-objects can follow from that. (If this doesn't make sense, read the example in #538.) The change looks a little silly right now, since it's this config object with one parameter, but it's an incremental path. There are a lot of legacy wrappers included which instantiate the default Config object, so I don't have to go change every top-level caller (in which case I'd likely break something because I'm not familiar with much of the code), but over time callers ought to be changed to explicitly pass a Config object, and the legacy wrappers can go away. * initialize with a fixed, not random, position offset random seems preferable for broader test coverage, but inference can be very sensitive to this, as on the `simple_star` test case: we infer probability of star = .005 when initializing with exactly the true position, and 0.995 when initialized slightly offset from the true position. (that's problematic by itself, but it's a different discussion.) since inference is sensitive, let's not randomize this offset and risk having extremely brittle tests (i.e., even if we srand, if some computations are reordered the random offset would change and the test could mysteriously break). * clean up some imports in tests These test files aren't under the Celeste module so they shouldn't really be doing .. imports, I think. It works because they're included into the runtests.jl script which imports a bunch of stuff, but it seems confusing, prone to breakage, and it doesn't work when you pull code out into sandbox scripts (for use with `reload()`).
) * add a logger that includes exception traceback, and wrap `Thread.@threads` loops with it * `Log.exception()` is like `Log.error()`, but includes exception information. Inspired by Python's `logging.exception()` and such. * Since exceptions inside threads are silently swallowed right now (JuliaLang/julia#17532), always wrap the body of a `Thread.@threads` call with a try-catch that manually logs the exception. * fix bug where i forgot to add the initial position offset * add Config object to faciliate passing params deep into the call chain As discucsed in PR #538, we have this problem of passing arguments deep down the call stack of Celeste inference. This manifests itself as lots of chains of keyword arguments and various not-so-pretty hacks (like custom `infer_source_callback` wrappers) to set options. The discussion in #538 outlines a broad direction for not only solving this problem, but also the related problem of different branches of code for different options at each stage of inference. The two current examples are single vs. joint inference and MOG vs. FFT PSF inference, but there will likely always be such branches. I still think that's a good direction to go, but even a minimal implementation is too invasive to be safe/worthwhile right now. Instead, this commit takes what I view as the simplest possible step in the right direction. We add a single `Config` object which is instantiated at top-level and passed as the first argument through all major function calls. Over time this object ought to absorb more parameters. Once there are distinct groups of parameters, the object can be split into multiple objects, and functions can receive only the sub-Config-object they require. Dynamic dispatch of different types of sub-Config-objects can follow from that. (If this doesn't make sense, read the example in #538.) The change looks a little silly right now, since it's this config object with one parameter, but it's an incremental path. There are a lot of legacy wrappers included which instantiate the default Config object, so I don't have to go change every top-level caller (in which case I'd likely break something because I'm not familiar with much of the code), but over time callers ought to be changed to explicitly pass a Config object, and the legacy wrappers can go away. * initialize with a fixed, not random, position offset random seems preferable for broader test coverage, but inference can be very sensitive to this, as on the `simple_star` test case: we infer probability of star = .005 when initializing with exactly the true position, and 0.995 when initialized slightly offset from the true position. (that's problematic by itself, but it's a different discussion.) since inference is sensitive, let's not randomize this offset and risk having extremely brittle tests (i.e., even if we srand, if some computations are reordered the random offset would change and the test could mysteriously break). * clean up some imports in tests These test files aren't under the Celeste module so they shouldn't really be doing .. imports, I think. It works because they're included into the runtests.jl script which imports a bunch of stuff, but it seems confusing, prone to breakage, and it doesn't work when you pull code out into sandbox scripts (for use with `reload()`).
This gets the GalSim unit tests passing again, per #537.
This is the simplest implementation but I think adding all these keyword args is ugly and not sustainable. First, it's just ugly to have a web of kw args spreading through the code. Second, because you want to make it optional, but don't want to duplicate the default value so many times, you end up doing all this Nullable business, which isn't particularly pretty in Julia. (You could alternatively make a global const holding the default value but I find that ugly too for all the global references.)
I don't know of a good clean alternative (setting global flags is a bad idea in my opinion) other than trying to solve the problem more holistically. There's just no easy way to pass options deep down into the call tree from the top level, that I know of. One possibility would be to make a big Configuration type, which holds options like this for the entire Celeste engine. I think it'd be best structured as a nested set of types, with a structure roughly mirroring the high-level modules of the Celeste code. (I'm using "modules" informally, not implying it has to match the Julia module structure of Celeste exactly.)
For example, maybe three modules of the Celeste code are 1) image preparation (choosing patches, active pixels, etc), 2) multiple source handling (single/joint inference & threading), 3) the inference method (MOG, FFT). (This is probably not a good/accurate separation but bear with me for the sake of example.) You might have something like
Some notable things:
infer_source()
andinfer_source_fft()
) and explicitly dispatching with conditionals or callback params (e.g.,infer_source_callback
), we dispatch a single entry point method call on the type of the config object, for each module.This is benefits but it's not simple, which is why I coded the "simple" approach in this PR so you can see it. Of course we don't need a mass conversion, this could be implemented somewhat piecemeal.
I also admit this is inspired by how I'd structure such a thing in an OO paradigm, so I hope there's a simpler, equally beneficial and more idiomatic solution in the Julia paradigm which I'm not thinking of, if so let me know!