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

ImageStreamIOWrap in Python fails on C-order arrays in confusing way #22

Open
joseph-long opened this issue Feb 24, 2022 · 5 comments · May be fixed by #53
Open

ImageStreamIOWrap in Python fails on C-order arrays in confusing way #22

joseph-long opened this issue Feb 24, 2022 · 5 comments · May be fixed by #53

Comments

@joseph-long
Copy link
Contributor

We just installed the dev branch on MagAO-X for testing and found what might be a regression.

import ImageStreamIOWrap
img = ImageStreamIOWrap.Image()
img.open('test')
import numpy as np
to_arr = np.array(img)  # works
x = img.copy() # works
img.write(x) # works

new0 = np.ones(x.shape, dtype=x.dtype)
img.write(new0)  # fails
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Input In [31], in <module>
----> 1 img.write(new0)

ValueError: incompatible type

It appears that this has something to do with C-contiguous vs. Fortran-contiguous arrays:

x.flags

  C_CONTIGUOUS : False
  F_CONTIGUOUS : True
  OWNDATA : True
  WRITEABLE : True
  ALIGNED : True
  WRITEBACKIFCOPY : False
  UPDATEIFCOPY : False
new0.flags

  C_CONTIGUOUS : True
  F_CONTIGUOUS : False
  OWNDATA : True
  WRITEABLE : True
  ALIGNED : True
  WRITEBACKIFCOPY : False
  UPDATEIFCOPY : False

@kvangorkom reported and I helped debug. Not sure where to go from here, but we can use fortran order explicitly as a workaround in the meantime.

cc @jaredmales

@DasVinch
Copy link
Member

Is this still current? I can't reproduce, but then it might depend on the shape and dtype of your 'test' image.

Preferred way, albeit not indispensable, is to use pyMilk for accessing ImageStreamIO from python (one more layer, but more convenience functions).
PyMilk goes around this problem by opening a "transpose" of the image by default. Which you can override into a F-ordered non-transpose if you prefer.

@a-sevin
Copy link
Member

a-sevin commented Aug 4, 2023

Sorry for the latency, this can be easily by removing the , py::array::f_style | py::array::forcecast of the write method. I encourage this change :) (old feature that we don't use anymore)

void write(IMAGE &img,
py::array_t<T, py::array::f_style | py::array::forcecast> b) {

@a-sevin a-sevin linked a pull request Aug 4, 2023 that will close this issue
@a-sevin a-sevin linked a pull request Aug 4, 2023 that will close this issue
@DasVinch
Copy link
Member

DasVinch commented Aug 4, 2023

Noted thanks. I'll justed have to do a synced update in pyMilk to force C ordering instead of F, and open without the transpose symcode.

I'm wondering if the enforced f_style isn't necessary relative to the fact that ImagestreamIO loves to straight up load fits (F ordered) and a lot of milk is based on that implicit assumption.
While it doesn't matter on a fundamental level (and obviously, C > F), I'm wondering if those implicit assumptions would end up being broken in a number of place where pyMilk talks to Cacao.

Just concerned. Possibly for no reason :)

@a-sevin
Copy link
Member

a-sevin commented Aug 5, 2023

I'm almost sure that astropy writes FITS files with the correct order, Eric saves a tone of FITS with python and he can use DS9 to read them without issue... But a real verification may be useful, indeed.

@DasVinch
Copy link
Member

DasVinch commented Aug 6, 2023

Astropy sure.

The idea is to establish the default symcode such that numpy -> pymilk -> milk's fits2shm is also equivalent to what we'd expect, without causing 12 copies along the way :)
We're just one test away from being sure we're happy, anyway.

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 a pull request may close this issue.

3 participants