Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

frebin and fshift functions #87

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Michael-Reefe
Copy link

I've added implementations of the frebin and fshift functions. I've tested a few cases and it seems to all agree with the IDL version. frebin requires the Interpolations.jl package as a new dependency.

@cgarling
Copy link
Member

cgarling commented Oct 3, 2024

I'm not an IDL guru, but I don't see fshift in the IDL astrolib function list here or on the github here. Can you provide a link to the documentation for the original IDL function?

I generally support adding new functions to this library, so thanks for the PR! It is currently JWST proposal season so I don't have time to do a full code review at the moment, but I noticed a few small things you could improve:

  • To enable automerge for package updates on the Julia General Registry, you need to add an upper bound for the version of Interpolations.jl to the compat section of the Project.toml (see more details here). I settled on 0.13.4 in my last project but don't remember exactly why.
  • You should probably state somewhere in the docstring of frebin what the valid dimensionalities for the input image are, since you are dispatching on AbstractArray which can have arbitrary dimensionality, but it looks like the code only works for 1 or 2D? To enforce this in the code you could do a check on the dimensionality of the input as ndims(image) and error if it does not equal 1 or 2, or use an assertion like @assert ndims(image) == 1 || ndims(image) == 2.

@Michael-Reefe
Copy link
Author

Thanks for taking the time to look at this during busy proposal season! The original IDL fshift function is archived here: https://asd.gsfc.nasa.gov/archive/idlastro/ftp/contrib/malumuth/fshift.pro. I also updated the compat requirements and dimensionality checking as suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants