Skip to content

Adds jittable Image#16

Merged
ismael-mendoza merged 27 commits intomainfrom
image
Jun 20, 2023
Merged

Adds jittable Image#16
ismael-mendoza merged 27 commits intomainfrom
image

Conversation

@EiffL
Copy link
Member

@EiffL EiffL commented Jun 11, 2022

This PR should be rebased on main once #15 is merged, it adds an implementation of galsim.Image.

There are some questions about this though. galsim.Image are mutable objects, which really does not play nicely with JAX, it is simply not possible to assign values to array in jax without making a copy. We can follow the galsim API, but the behavior will be different, it won't be possible to assign values to a view of a larger image.

In particular, we cannot do the following in Jax-GalSim:

gal = galsim.Gaussian(sigma=1)
im = galsim.Image(nx=128, ny=128, scale=0.1)

imview = im.view()

gal.drawImage(imview)

This would not update the original im.

This will become even sharper of a question when we go to the next PR, which implements drawing functions.

Already here, there is a question of whether we want to make all Image immutable explicitly, whether we want to allow this sort of things:

image = galsim.Image(nx=128, ny=128, scale=0.1)
image.wcs = galsim.PixelScale(0.2)
image.setCenter(0.0, 0.)

these are examples of bad OO patterns that do not play nicely at all with the JAX functional spirit, but if we remove them, we break the GalSim API....

@EiffL EiffL mentioned this pull request Jun 11, 2022
16 tasks
@EiffL EiffL changed the base branch from main to bounds June 14, 2022 23:06
@EiffL EiffL marked this pull request as ready for review June 14, 2022 23:07
@EiffL EiffL added this to the demo1.py milestone Jun 14, 2022
@EiffL EiffL added the enhancement New feature or request label Jun 14, 2022
@b-remy b-remy self-requested a review February 14, 2023 14:55
@b-remy b-remy self-assigned this Feb 14, 2023
@ismael-mendoza ismael-mendoza self-requested a review March 28, 2023 13:40
@ismael-mendoza ismael-mendoza self-assigned this May 5, 2023
Copy link
Collaborator

@ismael-mendoza ismael-mendoza left a comment

Choose a reason for hiding this comment

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

preliminary comments

Copy link
Collaborator

@ismael-mendoza ismael-mendoza left a comment

Choose a reason for hiding this comment

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

Seems like there are several places where jitting will break

@ismael-mendoza
Copy link
Collaborator

ismael-mendoza commented Jun 5, 2023

Facing this problem with the _make_empty function where I believe making arrays out of arguments is not jittable/vmappable. Is there any workaround? The alternative is to enforce the only option for creating an image is to use the array but that will really break the API.

I guess we need to discuss how we want to allow the Image object to be vectorized.

@ismael-mendoza ismael-mendoza self-requested a review June 19, 2023 14:37
@ismael-mendoza
Copy link
Collaborator

ismael-mendoza commented Jun 19, 2023

ToDo:

  • cleanup explicit init so it's a class method
  • check tree methods are correct
  • remove possibility of empty image?
  • fix test_image_jax.py and check it passes
  • use same dtype as array if passed in if dtype is None

Questions

  • Still don't understand why empty images are bad
  • What should default dtype be?
  • What was the point of "enable x64" again?
  • Test now uses explicit init? (just like a user would)
  • What to do about error messages about 'int' / 'float' type not available? (if x64 off)
  • valid dtypes? jnp.float32?

@ismael-mendoza ismael-mendoza removed the request for review from b-remy June 20, 2023 11:43
Copy link
Collaborator

@ismael-mendoza ismael-mendoza left a comment

Choose a reason for hiding this comment

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

Tests are passing, see if CI runs ?

@ismael-mendoza ismael-mendoza changed the base branch from bounds to main June 20, 2023 16:33
@ismael-mendoza ismael-mendoza merged commit 05e0863 into main Jun 20, 2023
@ismael-mendoza ismael-mendoza deleted the image branch July 21, 2023 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants