SPHEREx PSF notebook update using ePSf - #364
Conversation
troyraen
left a comment
troyraen
left a comment
There was a problem hiding this comment.
I skimmed through it and left a few comments below.
|
|
||
| psf_cube_plane = tab[0]["zone_id"] | ||
| distance_min = tab[0]["distance"] | ||
| # Not sure why this was necessary |
There was a problem hiding this comment.
We should try to understand this and put a more informative comment here. I can help with that if needed.
There was a problem hiding this comment.
I think this is just to convert them to floats.
According to chatGPT, it suggests to just remove the comment and replace it with something like
# Convert to Python floats (accepts ints and NumPy scalar types).
Alternatively, we could do the test:
try:
x0 = float(x0)
y0 = float(y0)
except (TypeError, ValueError) as err:
raise TypeError("x0 and y0 must be real numbers.") from err
What do you think? For now I just did option 1 (remove comment and replace it)
There was a problem hiding this comment.
Good choice to go with option 1. Looking at how this gets called in the notebooks, it seems like the conversion shouldn't be necessary at all. But doing it doesn't hurt anything (just adds more lines of code 🙃 ).
| ```{code-cell} ipython3 | ||
|
|
||
| ``` |
There was a problem hiding this comment.
| ```{code-cell} ipython3 | |
| ``` |
There was a problem hiding this comment.
not sure what's the difference?
There was a problem hiding this comment.
Sorry, that's probably confusing if you're not used to how github does things. The suggestion was to remove the extra, empty cell. Looks like you already did!
troyraen
added
maintenance
jkrick
left a comment
jkrick
left a comment
There was a problem hiding this comment.
small changes requested, otherwise looks good.
|
All comments above taken into account. @troyraen please have another look at the Exception comment you posted above to see if that makes sense. To do:
|
3 participants
update to use of new ePSF.
Currently, data is not public, so the notebook is run on local files.
Leaving this as a draft PR until the data is public so it's not accidentally merged before then.