edwardberman / shopt Goto Github PK

Mean Squared Error Loss Function can handle NaN out of the box with Flux but the Relative Error Loss Function is acting more detrimental. NaN's replace all model stars with I(x,y) < -1000 as a way of denoting other Stars in the image (Source Extractor sets these values to -e32)

Hi,

Your JOSS paper is pretty well written and clear! I have some minor suggestions, including fixing a few typos:

• I would remove the Introduction title, leaving simply Summary;
• Typo: 'unprecendented' (line 35);
• Typo: ‘biasses’ (line 67);
• Typo: ‘emblamatic’ (line 76);
• For the State of the Field section: I wonder how ShOpt.jl would compare with PSFr (https://github.com/sibirrer/psfr), which is quite recent and has been used for JWST data;
• Beichman et al., 2012a and Beichman et al., 2012b are exactly the same paper.

The default naming of the output directory follows something like 2024-01-15T11:20:40.848, but : characters are forbidden in macos path names (and are replaced by the / when displayed in a file explorer). Hence I would suggest not to use these characters but - or _ instead.

The tool and paper look great! Here are some comments (mostly minor):

Summary

• 16: You say "...essentially point sources before their light passes through the atmosphere and telescope..." Obviously this is normally correct from the ground, but since this is specifically a JWST paper you may want to remove the atmosphere part, or change it to "...through the atmosphere (when observing from the ground) and telescope..."
• 23: Since you've restricted this to NIRCam for now, I'd add that in the last paragraph: "...tailored to JWST imaging" --> "...tailored to JWST NIRCam imaging"

Statement of Need

• 32: Don't need to redefine JWST here
• 33: Please put an "e.g.," in front of the COSMOS citation, NIRCam offers a lot more scientific opportunities than just that!
• 67: less --> fewer
• While I agree that figure 2 highlights the need for non-Gaussian PSF models, would it be possible to make this a 4-panel figure showing what happens if you use a 2D Gaussian? I know that people are doing this, and it may be nice to show how much larger the residuals are
• 131 x 131 pixels is very large...I would be surprised if your fitter is actually able to pull anything useful out at that scale. Even your Figure 2 is just showing 75 pixels, and already it's basically noise at the edge even on a log scale. Even at 20 pixels or so (radius) you should have almost all the flux, so I'm not sure if/why you would need to go all the way to 4-5 arc seconds. Contamination from other objects would be pretty common at that level as well.

State of the Field

• 87: spelling out PIFF (and PSFex for that matter) should go to the first mention in the previous section
• 95: Best to change "twenty-five years ago" to a year, so that in future years the line still makes sense
• 125: "takes up" --> "covers"

In the current state of the JOSS paper, I suggest to mention STARRED as another efficient PSF reconstruction technique, as it has been shown to outperform other programs like PSFex (mentioned in the paper) and PSFr (which would need to be mentioned as well), and uses high the just-in-time compiled and auto-diff library JAX.

Fitting a kolmogorov radial fit is taking unfeasibly long. I know that there is an added degree of difficulty in numerical integration but perhaps something else is causing a bottleneck? I have tried printing loss with every iteration and it seems to be working, but I am working it may never even converge enough to satisfy optim.jl's requirements.

I ran into the following issues when running ShOpt for the first time.

Bugs

• dataPreprocessing.jl assumes VIGNET exists in HDU 2 of a FITS file. This is a very common issue; it would be nice if ShOpt handled this gracefully.
• reader.jl looks for an extension that doesn't exist in the default truncated mode (3)
• Star cutouts are off-center in "comfort plot" cutouts produced during run. Is this systematic? It could be minor but if that off-centered star is fed to the pixel grid fitter, that could propagate into the model and cause real problems down the line.

Suggested enhancements

• It would be nice if one could supply other config files to ShOpt. Half the point of using configuration files is not having to change the same file for different use cases.
• Not a bug, but the polynomial degree of the analytic (Gaussian) profile fit should be a tunable parameter. At the very least, it should match the pixel grid fit polynomial degree.

Hi,

I am trying to reproduce Figure 2 of the paper, and while it appears visually appealing, it would be helpful to have some additional clarifications:

1. It would be beneficial if you could specify in the repository (e.g. README) or on the GitHub site the precise JWST data file you downloaded and whether any modifications were made to the default SExtractor configuration files.
2. Is it possible for users to generate Figure 2 by simply providing the exact catalog and using TutorialNotebook.ipynb, or would additional code be required?

Thanks a lot for your time!

This issue is part of the JOSS review process.

I have been following the installation instructions and running of the program on the test catalog. Here are some comments regarding the documentation.

• The part on how to compile Python libraries for use in Julia is not straightforward to follow. In particular, it is not clear that there are three, even four, ways to achieve the same thing. I advise to re-formulate this part, stating ahead that there are these X different ways (e.g., as a list), and guide the user to the right section, instead of having all the commands in a row.
• It is important that the command pyimport("astropy") should be run after re-loading the Julia REPL, so it would better to have this explicitely mentioned.
• Is the TutorialNotebook.ipynb made just for copy-pasting lines of codes into the Julia REPL, or can this be executed using Julia directly? This is not clear to me. Probably because this is the first time I run Julia code: it means that the documentation can be be improved.
• using Base.Threads is missing in TutorialNotebook.ipynb.

Shopt Currently produces robust Analytic and Pixel Grid fits most of the time. Additionally, on the chance that a fit fails, I have Shopt record such. I wanted to raise an issue as a self note to make sure not to include the failed instances in the plotting, because at the moment these failed fits are making good data seem indecipherable, at least, overshadowing what I would otherwise deem successful. I am assigning this issue to myself to fix.