search

GalSim-developers / GalSim

The modular galaxy image simulation toolkit. Documentation:
http://galsim-developers.github.io/GalSim/
Other
227 stars 107 forks source link

Improve checks in GSParams.checks #1152

Closed EiffL closed 2 years ago

EiffL commented 2 years ago

While working on https://github.com/GalSim-developers/JAX-GalSim/pull/7 @ismael-mendoza found that the following would trigger an exception, not correctly captured:

import galsim
galsim.GSParams.check(None, default="anything not a GSParams")

This small PR adds a check that the default is indeed a GSParams instance.

I would have added a test for this, but I couldn't find a file with dedicated gsparams tests, what would be a good place for that?

Also, if some more things come up while digging through galsim, I'm happy to either make individual PRs for fixes, or a grouped PR with several small fixes, as you would prefer.

rmjarvis commented 2 years ago

I'm not sure I see the necessity of this. This isn't a method that users typically (maybe ever) call, so I'm not sure we need it to be super hardened to bad user input. So I'm worried that this is merely a performance regression (it's called a lot in relatively low-level functions) without any actual improvement in user interface.

esheldon commented 2 years ago

profile :)

EiffL commented 2 years ago

Fair, it is true that places that use this function are using an existing gsobjet.gsparams for the default argument, which are themselves systematically checked at the creation of a new gsobject. Feel free to close this PR if you prefer saving this isinstance check. But still interested if you have a pointer to a where GSParams is tested in the test suite :-)

rmjarvis commented 2 years ago

The use of gsparams is tested in pretty much all of the places it can be used. But this particular function is never in a top-line test, since it's basically a common utility function that other functions use.

rmjarvis commented 2 years ago

I think I'm going to close this as unnecessary. But please do keep raising issues as you come across things you think might be wrong or non-ideal in the course of your Jax efforts.