added first iteration of focus_diverse_epsfs #180

gsanand · 2023-09-28T17:04:13Z

added first iteration of a new module called focus_diverse_epsfs

pllim

What is the deadline for this, so I can prioritize among my other work?

Some general comments from initial quick glance:

You need to hook up the API doc to package documentation, if it is not getting picked up already.
Avoid referring to your own directory in docs. Use things others can access like astroquery.mast calls if the file is available from MAST.
Do you plan to add unit tests?
Is that gateway URL static? What about the auth info? Should it be public like that (is it guarded against DDoS attacks)? Does it take additional auth tokens that are private? Do not put anything private here.
Are you handling network issues gracefully? What if your query returns a HTTP Error 500 and so on?
Is it appropriate to use print instead of throwing proper exception in a function?
Do you really have to roll your own interpolation algorithm or can you use an existing one from photutils, scikit-image, scipy, numpy, and so on?
dask is an undeclared dependency
scipy is only an optional dependency
Please arrange imports in the order of stdlib, blankline, third-party (and each block, do the import first before from, and then alphabetically)

gsanand · 2023-09-28T17:28:53Z

Hi @pllim, in terms of deadline, we don't have a strict timeline, but ~2 to 3 weeks out from now is fine.

I'll take a look at addressing some of your comments. regarding the DDoS protection/auth info, that is the public API ID and Key we have generated, and we have quotas in place to prevent excessive usage on the AWS API Gateway end of things.

gsanand · 2023-09-28T17:30:15Z

also we tried several existing interpolation algorithms but found we needed to write our own

acstools/focus_diverse_epsfs.py

jryon

Looking good so far. I'll test out functionality after I return from vacation (Oct 11).

jryon · 2023-10-02T20:44:54Z

acstools/focus_diverse_epsfs.py

+    # select if either choosing to provide inputs via a text file or list
+    if fromTextFile == True:
+        with open(input_list) as file:
+            lines = [line.rstrip() for line in file]


This is something @pllim may have more to say about, but I think you'll need some smart handling/errors for poorly-formatted input files.

jryon · 2023-10-02T20:45:48Z

acstools/focus_diverse_epsfs.py

+    Downloads the ePSF FITS file to download_location. The explicit return is 
+    the name of the downloaded FITS file.
+
+    """


Check docstrings of other acstools modules for preferred format/content.

jryon · 2023-10-02T20:53:06Z

acstools/focus_diverse_epsfs.py

+    if y > 2048:
+        y = y-2048
+        chip = "WFC1"
+


Make it very clear in the docstring/docs that the expected coordinates here are for the whole 4096x4096 frame, not per chip. I'm not 100% sure I agree with this choice, since I'd imagine most people will be choosing their desired PSF location in an FLT/FLC, which has separate arrays for each chip. Maybe there's a way to enable both full-frame and per chip coordinates?

pllim · 2023-10-05T00:39:29Z

@gsanand , can you please rebase? I just overhauled the CI stuff. Thanks!

git fetch upstream master
git rebase upstream/master

You might have to resolve conflicts or not. Then when you are sure things went well:

git push -f origin master

TST: NumPy 2.0 and Python 3.12 (spacetelescope#179)

jryon · 2023-10-16T21:01:29Z

acstools/focus_diverse_epsfs.py

+
+Examples
+--------
+


Thought I commented on this earlier, but maybe it was in the ISR. The examples below need import statements, and ePSFs in the interpolation examples is not defined.

yes it was in the ISR, but I should have made the changes to the module as well. will do along with the next commit.

jryon · 2023-10-16T21:07:33Z

I did some basic functionality testing and things are working well. I have a few comments:

The trailing forwardslash '/' appears to be relevant for the download location. I accidentally left it off at first and wondered where my download went. The filename had the intended directory as a prefix. I'd suggest you add a forwardslash to whatever directory the user inputs.
The progress bar seems a little clunky. In one test, it behaved about as expected, but in another it stayed at 0% for several seconds, then went straight to 100%.
Forgive me if this is discussed in your ISR and I forgot - are subarrays not supported, then, since this module requires an FLC? Could we use an FLT instead or Is there some other reason for not supporting subarrays?

gsanand · 2023-10-17T15:25:46Z

Hey @jryon, thanks for the comments!

I implemented a fix for this so hopefully it should not be an issue anymore.
yes the progress bar is not great sometimes (slow to update). I looked into it and it just seems to be an issue with the underlying code, which I can't fix. I can take out the progress bar if you think that might be the better option.
subarrays are indeed not supported (because of the support only for FLCs). I actually did not state this in the ISR before, so I just added a line saying as much.

jryon · 2023-10-17T16:47:56Z

yes the progress bar is not great sometimes (slow to update). I looked into it and it just seems to be an issue with the underlying code, which I can't fix. I can take out the progress bar if you think that might be the better option.

In this case, I'm fine with leaving it, since it is useful at least some of the time.

pllim · 2023-10-30T19:20:41Z

acstools/focus_diverse_epsfs.py

+setting subpixel_x and subpixel_y between 0.00 and 0.99.
+
+We strongly recommend you read ACS ISR 2018-08 by A. Bellini et al. and
+ACS ISR 2023-XY by G. Anand et al. (LINK WHEN LIVE) for more details 


@gsanand , is this ISR published yet?

@pllim we are waiting to post the ISR (until the module is live). once the ISR is up, then we can update the module with a link (easier to update the module, than the ISR).

pllim · 2023-10-30T20:08:08Z

acstools/focus_diverse_epsfs.py

+>>> P = interp_epsf(ePSFs, x, y, pixel_space = True, subpixel_x = 0.77, subpixel_y = 0.33)
+
+
+More details for these examples are provided in a Jupyter notebook, which can be found at LINK.


I am going to delete this because this package isn't responsible for keeping any notebook elsewhere up-to-date (in fact, notebooks tend to quickly break and become unmaintained). So you can mention it in your ISR but you do not have to mention it here.

pllim · 2023-10-30T22:58:57Z

Sorry, I had to create #183 because of reasons... should we continue there?

pllim · 2023-11-07T19:47:34Z

Closing in favor of #183. Thanks for your patience with me!

added first iteration of focus_diverse_epsfs

570bac4

gsanand requested review from pllim and jryon September 28, 2023 17:04

gsanand self-assigned this Sep 28, 2023

pllim reviewed Sep 28, 2023

View reviewed changes

acstools/focus_diverse_epsfs.py Show resolved Hide resolved

gsanand added 2 commits October 2, 2023 14:35

Add files via upload

8c3689d

made some changes based on initial feedback

2e2bada

jryon reviewed Oct 2, 2023

View reviewed changes

gsanand added 2 commits October 3, 2023 10:02

added .rst file for focus_diverse_epsfs

109ab0c

fixed remaining PEP8 issues

b920753

gsanand added 2 commits October 5, 2023 10:56

Merge pull request #1 from spacetelescope/master

56e56bd

TST: NumPy 2.0 and Python 3.12 (spacetelescope#179)

docstrings now better match other acstools modules

413dc84

jryon reviewed Oct 16, 2023

View reviewed changes

updates incl. docstrings, trailing / support, etc.

2cbb815

Merge branch 'spacetelescope:master' into master

06e112b

pllim reviewed Oct 30, 2023

View reviewed changes

pllim mentioned this pull request Oct 30, 2023

PR 180 + pllim edits #183

Merged

2 tasks

pllim closed this Nov 7, 2023

pllim mentioned this pull request Nov 13, 2023

TST: Ignore acspsf URL in linkcheck job #188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added first iteration of focus_diverse_epsfs #180

added first iteration of focus_diverse_epsfs #180

gsanand commented Sep 28, 2023 •

edited

Loading

pllim left a comment

gsanand commented Sep 28, 2023

gsanand commented Sep 28, 2023

jryon left a comment

jryon Oct 2, 2023

jryon Oct 2, 2023

jryon Oct 2, 2023

pllim commented Oct 5, 2023

jryon Oct 16, 2023

gsanand Oct 17, 2023

jryon commented Oct 16, 2023

gsanand commented Oct 17, 2023 •

edited

Loading

jryon commented Oct 17, 2023

pllim Oct 30, 2023

gsanand Oct 30, 2023 •

edited

Loading

pllim Oct 30, 2023 •

edited

Loading

pllim commented Oct 30, 2023

pllim commented Nov 7, 2023

		>>> P = interp_epsf(ePSFs, x, y, pixel_space = True, subpixel_x = 0.77, subpixel_y = 0.33)


		More details for these examples are provided in a Jupyter notebook, which can be found at LINK.


		Examples
		--------

added first iteration of focus_diverse_epsfs #180

added first iteration of focus_diverse_epsfs #180

Conversation

gsanand commented Sep 28, 2023 • edited Loading

pllim left a comment

Choose a reason for hiding this comment

gsanand commented Sep 28, 2023

gsanand commented Sep 28, 2023

jryon left a comment

Choose a reason for hiding this comment

jryon Oct 2, 2023

Choose a reason for hiding this comment

jryon Oct 2, 2023

Choose a reason for hiding this comment

jryon Oct 2, 2023

Choose a reason for hiding this comment

pllim commented Oct 5, 2023

jryon Oct 16, 2023

Choose a reason for hiding this comment

gsanand Oct 17, 2023

Choose a reason for hiding this comment

jryon commented Oct 16, 2023

gsanand commented Oct 17, 2023 • edited Loading

jryon commented Oct 17, 2023

pllim Oct 30, 2023

Choose a reason for hiding this comment

gsanand Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

pllim Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

pllim commented Oct 30, 2023

pllim commented Nov 7, 2023

gsanand commented Sep 28, 2023 •

edited

Loading

gsanand commented Oct 17, 2023 •

edited

Loading

gsanand Oct 30, 2023 •

edited

Loading

pllim Oct 30, 2023 •

edited

Loading