Incorporate det into sca by arunkannawadi · Pull Request #30 · DukeCosmology/roman_imsim

arunkannawadi · 2025-05-06T14:54:04Z

This is NOT yet reviewed and is NOT ready to be merged

arunkannawadi · 2025-05-14T19:17:41Z

@yuedong0607 could you move the changes related to the coadd level simulation to another branch? Is the work on incorporating detector physics done and ready for some testing?

arunkannawadi · 2025-05-20T09:09:57Z

-    sky_subtract: False
+    sky_subtract: True
+
+    dither_from_file: /hpc/home/yf194/Work/final_sims_2023/Roman_WAS_obseq_11_1_23_ditherlist.txt


This is not readable. I also have changes in the prism branch that make this optional.

Hi Arun, this file is needed by the persistence module to select the neighboring dithers. Any suggestions on how to make this more readable? Could you point me to the changes in prism branch that you have mentioned?

I just meant that this file in your home directory is readable only by you, not others.

arunkannawadi · 2025-05-20T09:11:13Z

+    #         sky_image /= roman.gain
+    #         sky_image.quantize()
+    #         image -= sky_image
+


I don't want us to use commenting out code as a way to keeping history. Git does that for us. Just delete these lines.

have removed most of the commented old codes

arunkannawadi · 2025-07-22T14:57:23Z

Is this ready for review? If so, can you rebase this branch on to the main branch, resolve the conflicts and mark it as "Ready for review". Does this run? Trying to run this branch gave me an error, while the main branch did not.

yuedong0607 · 2025-07-22T18:37:47Z

Is this ready for review? If so, can you rebase this branch on to the main branch, resolve the conflicts and mark it as "Ready for review". Does this run? Trying to run this branch gave me an error, while the main branch did not.

Arun, could you be more specific about what error message you got? I just did a test run via galsim was.yaml and it completed successfully (see attached figure). The test I did was under simdevel2 environment. Strangely only pip install -e . doesn't work and I need to do a python setup.py install --user instead.

And I rebased on to the main branch.

arunkannawadi · 2025-07-22T19:59:22Z

Ah, never mind. The skyCatalogs version in simdevel2 is outdated. We'll need to make some changes in this repo while updating our environment to a more recent version, but that's not something that needs to happen in this branch.

Also, I don't see this branch rebased. You've pulled in the main branch in here. Please see the CONTRIBUTING.md on the main branch on how to rebase.

arunkannawadi

I have passed through the code once and I've given my first round of comments so you could work on them right away. I will give another round of review with some additional science inputs.

arunkannawadi · 2025-07-31T15:20:08Z

+import galsim.roman as roman
+
+
+class background(roman_effects):


Could you use CamelCase for all class names? So this would read as follows

Suggested change

class background(roman_effects):

class Background(RomanEffects):

arunkannawadi · 2025-07-31T15:24:28Z

+import numpy as np
+import fitsio as fio
+import galsim.roman as roman
+from roman_imsim.effects import roman_effects


Suggested change

from roman_imsim.effects import roman_effects

import .roman_effects

and similarly elsewhere.

arunkannawadi · 2025-07-31T15:27:18Z

+from .utils import sca_number_to_file
+
+
+class interpix_cap(roman_effects):


Suggested change

class interpix_cap(roman_effects):

class IPC(roman_effects):

IPC or InterPixelCapacitance

arunkannawadi · 2025-07-31T15:29:12Z


+            # # [TODO]
+            # break
+


Remove these lines

arunkannawadi · 2025-07-31T15:30:10Z

+            model: lab_model
    sky_subtract: False

+    # dither_from_file: /hpc/home/yf194/Work/final_sims_2023/Roman_WAS_obseq_11_1_23_ditherlist.txt


Suggested change

# dither_from_file: /hpc/home/yf194/Work/final_sims_2023/Roman_WAS_obseq_11_1_23_ditherlist.txt

arunkannawadi · 2025-07-31T15:38:09Z

+import roman_imsim.effects as effects
+
+
+class roman_effects(object):


Suggested change

class roman_effects(object):

class RomanEffects:

or more accurately RomanInstrumentalEffects. Long names are fine if they are more accurate and descriptive.

arunkannawadi · 2025-07-31T15:40:31Z

+from .utils import sca_number_to_file
+
+
+class saturate(roman_effects):


Suggested change

class saturate(roman_effects):

class Saturation(RomanEffects):

Let's use nouns for all the effects.

arunkannawadi · 2025-07-31T15:43:54Z

+from .utils import get_pointing
+
+
+class setup_sky(object):


Should this live inside roman_imsim.effects? It's not an effect itself. This module should live one directory above.

arunkannawadi · 2025-07-31T15:50:29Z

+import galsim.roman as roman
+
+
+class recip_failure(roman_effects):


Suggested change

class recip_failure(roman_effects):

class ReciprocityFailure(RomanEffects):

arunkannawadi · 2025-07-31T15:51:03Z

+                "%s hasn't been implemented yet, the simple model will be applied for %s"
+                % (str(self.params["model"]), str(self.__class__.__name__))
+            )
+            self.model = self.simple_model


For effects that do not have a lab_model implemented, we need to emit a warning OR error saying that they are not yet implemented if they are set to lab_model in the config file.

…ects.py

* Enable selection of different models for each effect via the configuration YAML. * Implementing a nested class structure in detector_effects.py. * Testing the implementation of QE and BFE effects.

…g within its child classes.

…ects.py

* removing some commented-out codes * move setup_sky to RomanEffects file.

yuedong0607 · 2025-08-28T20:23:53Z

I ran both this branch and main using the following noise setup:
main:
stray_light: True
thermal_background: True
reciprocity_failure: True
dark_current: True
nonlinearity: True
ipc: True
read_noise: True

this branch:
add_effects:
Background:
model: simple_model
thermal_background: True
stray_light: True
ReciprocityFailure:
model: simple_model
DarkCurrent:
model: simple_model
Nonlinearity:
model: simple_model
IPC:
model: simple_model
ReadNoise:
model: simple_model
Gain:
model: simple_model

the only differences I noticed was the date parameter given to getSkyLevel() call (see issue #68)
Aside from that, the results from the two branches differ only slightly. Some statistics are shown below:
mean diff 0.0022401611704918408
median diff 0.0
std diff 23.382407151643644

yuedong0607 force-pushed the incorporate_det_into_sca branch from 68096c4 to 270869e Compare May 15, 2025 20:55

arunkannawadi commented May 20, 2025

View reviewed changes

yuedong0607 marked this pull request as ready for review July 22, 2025 19:23

yuedong0607 force-pushed the incorporate_det_into_sca branch from 2ae453f to 957f4e3 Compare July 30, 2025 23:27

arunkannawadi commented Jul 31, 2025

View reviewed changes

yuedong0607 added 21 commits August 27, 2025 01:23

rearrange functions in detector_physics.py to sca.py and detector_eff…

3e34f7b

…ects.py

fix clip calls in add_persistence

48b3560

remove some commented codes in sca.py

4540ad0

Start refactoring the detector_effects class:

9aef8d2

* Enable selection of different models for each effect via the configuration YAML. * Implementing a nested class structure in detector_effects.py. * Testing the implementation of QE and BFE effects.

Start refactoring the detector effects in a subfolder

2532065

remove debugging code

1f291cd

Add cross_refer method to roman_effects for internal cross-referencin…

23305b1

…g within its child classes.

add some logging to bfe and recip_failure

c560726

add dead_pix, interpix_cap, read_noise, vtpe

56e4962

add gain and bias model to effects

52d3cd7

add setup_sky class for generating sky as well as sky subtracted images

b0a8713

modify config and sca.py for generating sky subtracted images

cc56da0

rearrange functions in detector_physics.py to sca.py and detector_eff…

12b1c1e

…ects.py

add .gitignore file

c3b6474

rearrange functions in detector_physics.py to sca.py and detector_eff…

bab7272

…ects.py

rearrange functions in detector_physics.py to sca.py and detector_eff…

212c0bf

…ects.py

reformat to meet pep8

98ff4c3

black reformated

bb9168a

black reformated

f727aac

black formatted

7255582

* renaming the classes using CamelCase

a765773

* removing some commented-out codes * move setup_sky to RomanEffects file.

yuedong0607 added 4 commits August 27, 2025 01:23

move model validity checking into base (RomanEffects) class

7c6bf21

renamed the files for effects

24a84ed

fix the import in sca.py

fe411a8

bug fix in read_noise.py: missed assignment for simple model

0346889

yuedong0607 force-pushed the incorporate_det_into_sca branch from 1b39b5d to 0346889 Compare August 27, 2025 05:23

* remove duplicates in detector_physics.py

1ff7ee3

		import galsim.roman as roman


		class background(roman_effects):

	class background(roman_effects):
	class Background(RomanEffects):

	from roman_imsim.effects import roman_effects
	import .roman_effects

		from .utils import sca_number_to_file


		class interpix_cap(roman_effects):

		import roman_imsim.effects as effects


		class roman_effects(object):

		from .utils import sca_number_to_file


		class saturate(roman_effects):

	class saturate(roman_effects):
	class Saturation(RomanEffects):

		import galsim.roman as roman


		class recip_failure(roman_effects):

	class recip_failure(roman_effects):
	class ReciprocityFailure(RomanEffects):

Conversation

arunkannawadi commented May 6, 2025

Uh oh!

arunkannawadi commented May 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arunkannawadi commented Jul 22, 2025

Uh oh!

yuedong0607 commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arunkannawadi commented Jul 22, 2025

Uh oh!

arunkannawadi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuedong0607 commented Aug 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yuedong0607 commented Jul 22, 2025 •

edited

Loading