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

DM-34778: Expose interpolant in Piff config #12

Merged
merged 2 commits into from May 25, 2022
Merged

Conversation

arunkannawadi
Copy link
Member

No description provided.

@arunkannawadi arunkannawadi force-pushed the tickets/DM-34778 branch 6 times, most recently from 1fcb5f2 to 9895934 Compare May 24, 2022 04:10
@@ -32,6 +33,22 @@


class PiffPsfDeterminerConfig(BasePsfDeterminerTask.ConfigClass):
def _validateGalsimIntepolant(name: str) -> bool: # noqa: N805
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intepolant -> Interpolant

"""A helper function to validate the GalSim interpolant at config time.
"""
# First, check if ``name`` is a valid Lanczos interpolant.
pattern = re.compile(r"Lanczos\(\d+\)")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to let GalSim itself determine valid names here I think (to handle to off-chance that we add a new interpolant there, for example). You can use galsim.Interpolant.from_name() and look for GalSimValueError.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some difference in the way PiFF accepts interpolant names and GalSim does. For e.g., GalSim accepts sinc but PiFF needs it to be galsim.SincInterpolant. And galsim.Interpolant.from_name('sinc').__class__ returns galsim.Interpolant.SincInterpolant, which is not accepted by PiFF. I don't foresee using anything other than Lanczos, and since the doc says we support only the ones listed there, if there are additional interpolation schemes that get added in GalSim, we can add that later. We should consider it as a technical debt at this point the very least.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could mock up what PiFF does currently. We can do

import galsim
from galsim import Lanczos
...
def _validateGalSimInterpolant(name):
    try:
        eval(name)
        return True
    except AttributeError:
        return False

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting. Looks like Piff discourages use of anything non-Lanczos? Which is probably reasonable. I like this best.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thoughts, any executable code would pass this, even if it is not a valid interpolant name. And it'd fail later during runtime. Also, from a security standpoint, we don't want to execute an arbitrary code here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you could add an isinstance(blah, galsim.Interpolant) check.

As for security, I don't know. GalSim and piff both contain eval statements and we use them without worry. We store piff results in the stack as pickles and I'm pretty sure you can hide arbitrary instructions in those. I kinda think the ship has sailed with regard to python eval.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if PiFF discourages anything non-Lanczos, but it is expecting the user to be sensible. As GalSim users, I know you and I both tried to set it to Lanczos11 which is invalid. So I think it is fair to say that other users might likely set it incorrect at first, and I'd want to capture this during config validation stage. I think I'm going to fall back to the implementation I had earlier, for reasons I mentioned in the earlier comment. Sorry Josh :-|

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. That's fine.

@arunkannawadi arunkannawadi force-pushed the tickets/DM-34778 branch 4 times, most recently from d40fcbb to c612802 Compare May 24, 2022 16:49
jmeyers314 and others added 2 commits May 24, 2022 14:09
Co-authored-by: Josh Meyers <jmeyers314@gmail.com>
Co-authored-by: Josh Meyers <jmeyers314@gmail.com>
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.

None yet

2 participants