search

AstarVienna / ScopeSim

A telescope observation simulator for Python.
GNU General Public License v3.0
16 stars 10 forks source link

Don't apply detector array effects twice #489

Closed teutoburg closed 1 week ago

teutoburg commented 3 weeks ago

The detector array effects are currently applied twice in DetectorManager.readout(), as stated in the comment there:

        # FIXME: Why is this applied twice ???

This seems to have been added (c3140674206e4a754707c02256d16a7cbab97014) in an attempt to get the header keywords in after the detector effects. The corresponding TODO is however still there:

            # 6. add necessary header keywords
            # .. todo: add keywords

The problem is that the 900-range detector array effects (e.g. ExtraFitsKeywords) also include the two electronic effects AutoExposure and DetectorModePropertiesSetter, whereas the other electronic effects are all in the 800-range of detector effects. So currently, the order of effect application is 900-800-900.

One solution would be to introduce a new 1000-range of z_orders for the header keywords stuff, and maybe also swap the 800s and 900s, so the order of application is the right way around. Or, somewhat simpler, considering we're thinking about moving away from the z_order altogether in the long run, maybe split the 900 range in the OpticsManager into a 900 and a 950 range, with the lower one being the detector array effects and the higher one the header keywords. The latter solution should be relatively quick to at least try and see if it works...

hugobuddel commented 3 weeks ago

I propose to just go for the 1000-range z-order. That sounds simpler than hacking something in OpticsManager, because that is what the z_order is for right (even if we don't like it)? I'm probably missing something. But either way sounds fine.

teutoburg commented 3 weeks ago

IIRC the z_order is currently only relevant in the hundreds-place, i.e. the order of effects within each 100-range is determined only by their order of appearance in the respective YAML, while the second and third place of the z_order number is ignored. I think I once tried to change that an it didn't go well.

hugobuddel commented 3 weeks ago

Yes exactly, it sounds like that hacking something such that the z-order works on the 50-ranges for 9xx seems less maintainable than extending the z-range to 1100