More doc updates

[schlafly]

Closes #152.

I adopted most of the changes wholesale. Here is a list of some of the ones where more conversation might be beneficial:

• Overall, there is quite a bit of confusion throughout the documentation related to units. There is about a factor of 2 difference between electrons in the pixels/detectors and DNs in the read arrays, but both are usually referred to as ‘counts, which is a very generic term. Also, sometimes the word ‘photon(s)’ seems to be used instead of ‘electron(s)’, which make readers think the sentence is not describing the detector but the source. Please replace relevant instances of “photon(s)” and “count(s)” with the correct units (either electron(s) or DN(s)) according to the context. This will help clarify a lot of confusing statements.

We adopted electrons when referring to Poisson-distributed quantities, and DN when appropriate.

• Also, there are a few pages, mostly related to the description of L1 and L2 images, containing what appears to be a list of personal notes and thoughts rather than a description/documentation of what the relevant functions/methods do. We strongly suggest to rewrite those pages in the spirit of a documentation style, and avoid the use of “I” or “we” subjects as much as possible.

This was trimmed slightly and prefaced slightly, but I find some of that discussion useful and did not feel the need to just delete it.

• The “Search docs” bar does not work. This might not be a romanisim-only issue, as the issue appears to be spread out to most/all readthedocs pages, but it should still be fixed.

This looks to be fixed with a config change.

• In some pages equations are given using a latex interpreter while in others are simply typed down in ASCII, which are less easy to read. We suggest to always use the latex interpreter for clarity.

We updated these when called out in the detailed notes.

• Sometimes GalSim is rendered as ‘GalSim’, other as ‘galsim’ (even in the same page, e.g., https://romanisim.readthedocs.io/en/latest/romanisim/wcs.html). Also, WebbPSF is always rendered as ‘webbpsf’. Can these packages be consistently rendered throughout the documentation? If a distinction is made to distinguish between GalSim as the name of the program, and galsim as the python package, can a different font be used for python packages, to improve clarity?

I haven't changed the styling here.

• 'This functionality is not available to the general community at the time of writing.' It looks like this sentence can be removed now, as CRDS is public and people should really to use --usecrds to create realistic images. In addition, --usecrds=True should be the default behavior, since otherwise galsim outputs wrong images (x-flipped).

I've deleted the line about CRDS not being public. I have not changed the default; the present default matches GalSim's default, which is e.g. what was used for the large PIT simulations, and CRDS has its own issues (e.g., the linearity reference files are of marginal quality).

• The one to the last sentence starting with 'You may wish to, for example, set up a new python virtual environment' should be put near the beginning of the page, right before pip installing romanisim. Reason: some users have reported conflicts with unidentified python packages, and a clean environment appeared to fix all of them. A clean environment must be set up before any pip command is executed.

This is general python stuff and has no particular relationship to romanisim; I have not made a change.

https://romanisim.readthedocs.io/en/latest/romanisim/running.html
Please update the 'romanisim-make-image -h' output with the one produced in the latest version of romanisim. The output currenly on the page still mentions 'fits' instead of 'asdf', for instance, and input catalogs in .csv format made with romanisim do not work.

• ‘The --webbpsf argument indicates that the WebbPSF package should be used to simulate the PSF; note that this presently disables chromatic PSF rendering.’ Later on page https://romanisim.readthedocs.io/en/latest/api/romanisim.image.add_objects_to_image.html, there is this statement: ‘Note: this includes Poisson noise when photon shooting is used (i.e., for chromatic source profiles), and otherwise is noise free’. As it is, it seems that there is no Poisson noise if users have the —webbpsf tag on? Please clarify.

add_objects_to_image does not add Poisson noise if --chromatic is false, and so that nois emust be added at other stages of the simulation. We do this, e.g., in L2, L3, L2 injection, and L3 injection. I have not changed the text.

• ‘for the rate of photons ‘. The sentence describes up-the-ramp fitting, so it should really be DNs not photons. Please check the unit.

I haven't changed this. We convert the DN to electrons and then do the fit, in the same way as the pipeline. The result is then converted back to DN / s for the final L2 images using the gain.

• ‘dark rate image to subtract from ramps (electron / s)’. Dark rate files in DRCS are in DN/s. Please clarify and fix the sentence. Also make sure that romanisim uses the correct unit for dark-rate files from CRDS.

I haven't changed this. romanisim converts the dark from DN / s to e / s before sending it to this routine.

• 2.Replace ’0, 1 or 2, specifying level 1 or level 2 image 0 makes a special idealized ‘counts’ image’ with ‘0, 1 or 2, specifying level 1 or level 2 image 0 makes a special idealized DN image’ to avoid confusion. Reason: L1 and L2 are in DN and DN/s and “Counts’ it too generic and confusing. Unclear what the unit the Level-0 image actually is, as there is little to no documentation about these level-0 data. We suggest to either add documentation to describe level-0 data, or not mention level 0 at all to avoid confusion.

I've added that the level 0 images are only intended for testing purposes, and changed counts to "total electron".

• ‘This gives the total counts in an idealized instrument with no systematics;’. Isn’t this a realization of the total counts taken from a Poisson distribution, as described in https://romanisim.readthedocs.io/en/latest/romanisim/image.html? If true, this function will never actually produce an image with the total counts of sources, but only a possible Poisson realization of them. Please clarify. Also, explicitly state the unit instead of ‘counts’.

I have changed total counts to total electrons. I have added that this includes Poisson noise.

• Replace ‘dark rate image to use (electrons / s)’ with ‘dark rate image to use (DN/ s)’. Reason: the dark rate image in CRDS is in units of DN/s. If the dark rate is converted from DN/s to e/s via the gian, please state it in the documentation.

This input takes the dark rate image in electrons / s. The conversion from the native CRDS units to the dark rate is done elsewhere in the simulation.

• ‘do not render sources more than this many pixels off edge of detector’. Does this imply that light from sources outside the SCA can still contribute to the image? This seems to contradict text in https://romanisim.readthedocs.io/en/latest/api/romanisim.image.in_bounds.html. Please clarify.

This is exactly the argument "margin" passed to in_bounds. The docstring for in_bounds has been modestly updated.

• ‘In contrast to inbounds, this doesn’t require the x and y coordinates of the sources, and just uses the ra/dec directly without needing to do the WCS transformation’. Does this mean that the trim is done before considering distortion? If so, hopefully the 10% extra distance is enough to account for distortion effects in all SCAs. Was this checked?

I've updated this text. The point is that this doesn't get each source's x & y coordinates, and instead just gets everything in a circle with a radius of 1.1 times the center-corner distance. The center-corner distance uses the full WCS and distortion. I have not checked that this is always adequate, but I would be surprised if it were not. Let me know if you find otherwise.

• ‘the pixels count the total number of photons having entered each pixel’. Please check that the unit (photons) is correct. If the point of view is the read, it should be DN. if the point of view is a pixel, then it should be electrons, which is what pixels store. If indeed the point of view are the sources themselves, then the sentence needs to be made clearer.

I've changed to electrons.

• ‘which contains the number of photons each pixel’. Please check the unit. Pixels in the detector contain electrons. Pixels in a read image contain DN.

I've changed to elecetrons.

• ‘drawing the appropriate number of photons from the total number of photons for each read following a binomial distribution’. Make sure photons is the correct unit. Pixels in the detector store electrons. Pixels in the read image are in DNs.

Changed to electrons.

• “This is slightly different than including IPC in the PSF kernel because including IPC in the PSF kernel leaves the Poisson noise uncorrelated’. The pixel value in read N does depend on the pixel value of read N-1, as reads are nondestructive, so there is still correlation regardless of where IPC is applied (or even not applied).

I added "between pixels." Of course the correlation between reads and resultants doesn't depend on IPC, but IPC correlates one pixel with its neighbors.

• Replace ‘If the same pixel is receives’ with ‘if the same pixel receives’. Reason: typo/grammar. On the same sentence: ‘‘If the same pixel is receives large fluxes multiple times, these are treated as two independent events’. Does this mean that multiple events are really treated as just 2 independent events? If so, why, please explain. If not, please fix the text.

The point here is that in past exposures the same pixel can be hit by a bright source multiple times. e.g., a bright star in the previous exposure and the two-previous exposure. Sanchez+2023 doesn't have a particular model for what the persistence flux is as a function of time for that case. In the simulation we model this as the persistence flux that would be left over from the exposure two exposures ago plus the persistence flux that would be left over from the exposure one exposure ago. I have slightly tweaked the text.

• Replace ‘appropriate assignment of counts to each read composing a resultant’ with ‘appropriate assignment of DNs to each read composing a resultant’. Reason: DN is the correct unit here.

Changed to electrons. This phase of the simulation is working with electrons.

• Replace ‘This apportions the total counts ‘ with ‘This apportions the total DNs ‘. Reason: DN is the correct unit here.

Replaced with electrons, because this is what is done.

• Throughout the page, there are a lot of philosophical arguments about what should and should not be done when fitting ramps, and what should work best for what case. All of this can simply be removed in favor of just describing what the module actually does.

I have left in most of this discussion.

• The dark files in CRDS already contain a 2D array of the dark rate image in units of DN / s. Why is a scaling of the last resultant by its effective exposure time used, instead?

This was out of date and has been replaced; the documentation was from before the dark rate column existed.

• ‘galsim.roman has an implementation of Roman’s WCS based on some SIP coefficients for each SCA. This is presumably plenty good’. It might be plenty good for some applications, but not for others. This remark should be removed as it is not universally true. In addition, galsim appears to produce wrong output images (x-flipped) when CRDS is not used.

Changed to "could presumably be adequate."

• https://romanisim.readthedocs.io/en/latest/romanisim/parameters.html

I did not add the default values for all of these quantities, since I imagine they may change. I did add a link to the code where they are all defined. In some cases where I found the values very unlikely to change I added specific values.

• ‘the number of border pixels’. Is this about reference pixels? If so, replace ‘border’ with ‘reference’. Also, is it just the number of them in the parameter module, or also their geometry?

I changed this to the number of border reference pixels (4).

• https://romanisim.readthedocs.io/en/latest/api/romanisim.util.scalergb.html
• Please fix the dimension order of numpy arrays: F[z,y,x] not F[z,x,y]. Similar for F[y,x] not F[x,y].

Here I've changed the docstring to be np.ndarray[ny, nx, 3] from np.ndarray[npix, npix, 3]. I don't think it's a good idea to describe the last axis, which has the three color channels, as the x axis.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.07%. Comparing base (81028a8) to head (9ef3f81).
Report is 54 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
+ Coverage   89.24%   91.07%   +1.83%     
==========================================
  Files          17       16       -1     
  Lines        2073     2118      +45     
==========================================
+ Hits         1850     1929      +79     
+ Misses        223      189      -34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[schlafly]

@AndreaBellini, let me know if you're interested in looking over these doc changes before I merge.

[schlafly]

Andrea signed off out of band, so I'm going ahead and merging this.