roman-corgi / corgidrp

Data Reduction Pipeline for the Roman Coronagraph Instrument
BSD 3-Clause "New" or "Revised" License
5 stars 4 forks source link

Move non-calibration parameter inputs in functions to detectorParams instead? #126

Closed kjl0025 closed 4 months ago

kjl0025 commented 5 months ago

Would it be better to house all such parameters in DetectorParams instead of having some as kwargs in different step and calibration functions? Can the walker handle both cases? As long as kwargs and DetectorParams can be tweaked by the CTC once the DRP is in Docker, I suppose either way would be fine.

maxwellmb commented 5 months ago

I think it depends on what you mean by "parameter". Recipe templates can change keyword arguments to step functions. If I understand correctly, we should be able to tweak the recipe templates without a code review since they're held in .json files, and not .py files. It would be good to have this confirmed by @mygouf and/or Cynthia.

The way that I've been thinking about things:

We could better codify this in the README as well. @semaphoreP so these definitions make sense to you too?

JuergenSchreiber commented 4 months ago

I think it depends on what you mean by "parameter". Recipe templates can change keyword arguments to step functions. If I understand correctly, we should be able to tweak the recipe templates without a code review since they're held in .json files, and not .py files. It would be good to have this confirmed by @mygouf and/or Cynthia.

The way that I've been thinking about things:

  • Constants - things that are highly unlikely to change - can be included in modules, like detector.py
  • Properties of the system - things we need to measure that might change in time - would go in the calibration
  • Function parameters - choices that we might make and want to have more flexibility (e.g. exactly what area of a detector we might use to calculate something) - would be keyword arguments

We could better codify this in the README as well. @semaphoreP so these definitions make sense to you too?

@maxwellmb , to clarify, which one would you prefer to go into DetectorParams, Constants I guess?

maxwellmb commented 4 months ago

I meant "Properties of the system" that need to be measured and might change in would go into a calibration file like DetectorParams.

semaphoreP commented 4 months ago

I agree with this philosophy. Are there any arguments that should be changed?

maxwellmb commented 4 months ago

@kjl0025 Do you feel like this issue can be closed now? Perhaps the remaining task is to add the definitions above to the readme?

kjl0025 commented 4 months ago

Yes, I am fine with closing this issue as long as the README is adjusted. Since the walker can handle kwargs, then it does kind of make sense to keep function parameters associated with a particular step function coupled to them via kwargs (e.g., the cosmic threshold values used in the cosmic ray detection function).

maxwellmb commented 4 months ago

Sounds good. I updated the README. Closing issue