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

Should erfa ufuncs accept correctly structured input independent of field names? #77

Open
mhvk opened this issue May 22, 2021 · 7 comments

Comments

@mhvk
Copy link
Contributor

mhvk commented May 22, 2021

EDIT: note it is not completely obvious what is right, since it may not be so easy to ensure that, e.g., erfa.cpv produces output with the same field names as the input.

Right now, the erfa functions insist on the correct field names:

In [14]: pv1 = np.array([([0.,1.,2.], [3.,4.,5.])], dtype=np.dtype([('p', '3f8'), ('v', '3f8')]))

In [15]: pv2 = np.array(([5.,4.,3.], [2.,1.,0.]), dtype=np.dtype([('pos', '3f8'), ('vel', '3f8')]))

In [16]: erfa.cpv(pv1)
Out[16]: 
array([([0., 1., 2.], [3., 4., 5.])],
      dtype={'names':['p','v'], 'formats':[('<f8', (3,)),('<f8', (3,))], 'offsets':[0,24], 'itemsize':48, 'aligned':True})

In [17]: erfa.cpv(pv2)
TypeError: ufunc 'cpv' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''same_kind''

Yet, numpy works by position and allows e.g. the following:

In [18]: pv1[:] = pv2

In [19]: pv1
Out[19]: 
array([([5., 4., 3.], [2., 1., 0.])],
      dtype=[('p', '<f8', (3,)), ('v', '<f8', (3,))])
@mhvk mhvk changed the title erfa ufuncs should accept correctly structured input independent of field names Should erfa ufuncs accept correctly structured input independent of field names? May 22, 2021
@mhvk
Copy link
Contributor Author

mhvk commented Jun 3, 2021

@seberg - sorry to ping you outside of numpy, but in the C API, is there currently a way to ask whether two structured dtypes have the same structure but possibly different names? Right now, we use PyArray_EquivTypes but that checks names too; I do want to ensure that both types are structured, i.e., not allow float->structured.

@seberg
Copy link

seberg commented Jun 3, 2021

No, I don't think there isn't. We still need to fix the casting safety for structured dtypes, but I don't think that would help you.

I think the only way is to access descr->fields directly.

@mhvk
Copy link
Contributor Author

mhvk commented Jun 3, 2021

OK, thanks. So that is what happens in things like __setitem__ too?

@seberg
Copy link

seberg commented Jun 4, 2021

Yes, it uses this code/pattern (this is part of the casting code):

    for (i = 0; i < field_count; ++i) {
        key = PyTuple_GET_ITEM(dst_dtype->names, i);
        tup = PyDict_GetItem(dst_dtype->fields, key);
        if (!PyArg_ParseTuple(tup, "Oi|O", &dst_fld_dtype,
                                                &dst_offset, &title)) {
            NPY_AUXDATA_FREE((NpyAuxData *)data);
            return NPY_FAIL;
        }
        /* identical code for the source here */

Not sure about iterating the dictionary in C, but maybe it always ordered in any case (since the name/key is only used to fetch the field, that step might just be unnecessary).

I guess this is effectively public API, so I think you can just copy the pattern.

@seberg
Copy link

seberg commented Jun 4, 2021

Actually, the problem here is just the same_kind casting after all?! So, if we would modify field renames to be considered "same-kind", this would just work without any extra work on your side?

That would seem better to me, since this whole custom type resolution is pretty iffy, especially for casting safety. (I realize that fixing the casting safety has been on the "Not Done" list for too long, but...)

@mhvk
Copy link
Contributor Author

mhvk commented Jun 4, 2021

Yes, I'd love same_kind to just work for the same structured dtype irrespective of field names. Indeed, I think 'equiv' should perhaps be OK as long as all the fields are equivalent?

@seberg
Copy link

seberg commented Jun 4, 2021

I think we should go with "safe" rather than "equiv", but yeah. That seems fine to me, and its probably enough of a mess that we can just change it.

seberg added a commit to seberg/numpy that referenced this issue Jun 11, 2021
This PR replaces the old numpygh-15509 implementing proper type promotion
for structured voids.  It further fixes the casting safety to consider
casts with equivalent field number and matching order as "safe"
and if the names, titles, and offsets match as "equiv".

The change perculates into the void comparison, and since it fixes
the order, it removes the current FutureWarning there as well.

This addresses liberfa/pyerfa#77
and replaces numpygh-15509 (the implementation has changed too much).

Fixes numpygh-15494  (and probably a few more)

Co-authored-by: Allan Haldane <[email protected]>
seberg added a commit to seberg/numpy that referenced this issue Jun 26, 2021
This PR replaces the old numpygh-15509 implementing proper type promotion
for structured voids.  It further fixes the casting safety to consider
casts with equivalent field number and matching order as "safe"
and if the names, titles, and offsets match as "equiv".

The change perculates into the void comparison, and since it fixes
the order, it removes the current FutureWarning there as well.

This addresses liberfa/pyerfa#77
and replaces numpygh-15509 (the implementation has changed too much).

Fixes numpygh-15494  (and probably a few more)

Co-authored-by: Allan Haldane <[email protected]>
seberg added a commit to seberg/numpy that referenced this issue Jun 26, 2021
This PR replaces the old numpygh-15509 implementing proper type promotion
for structured voids.  It further fixes the casting safety to consider
casts with equivalent field number and matching order as "safe"
and if the names, titles, and offsets match as "equiv".

The change perculates into the void comparison, and since it fixes
the order, it removes the current FutureWarning there as well.

This addresses liberfa/pyerfa#77
and replaces numpygh-15509 (the implementation has changed too much).

Fixes numpygh-15494  (and probably a few more)

Co-authored-by: Allan Haldane <[email protected]>
seberg added a commit to seberg/numpy that referenced this issue Feb 24, 2022
This PR replaces the old numpygh-15509 implementing proper type promotion
for structured voids.  It further fixes the casting safety to consider
casts with equivalent field number and matching order as "safe"
and if the names, titles, and offsets match as "equiv".

The change perculates into the void comparison, and since it fixes
the order, it removes the current FutureWarning there as well.

This addresses liberfa/pyerfa#77
and replaces numpygh-15509 (the implementation has changed too much).

Fixes numpygh-15494  (and probably a few more)

Co-authored-by: Allan Haldane <[email protected]>
seberg added a commit to seberg/numpy that referenced this issue Feb 24, 2022
This PR replaces the old numpygh-15509 implementing proper type promotion
for structured voids.  It further fixes the casting safety to consider
casts with equivalent field number and matching order as "safe"
and if the names, titles, and offsets match as "equiv".

The change perculates into the void comparison, and since it fixes
the order, it removes the current FutureWarning there as well.

This addresses liberfa/pyerfa#77
and replaces numpygh-15509 (the implementation has changed too much).

Fixes numpygh-15494  (and probably a few more)

Co-authored-by: Allan Haldane <[email protected]>
seberg added a commit to seberg/numpy that referenced this issue May 5, 2022
This PR replaces the old numpygh-15509 implementing proper type promotion
for structured voids.  It further fixes the casting safety to consider
casts with equivalent field number and matching order as "safe"
and if the names, titles, and offsets match as "equiv".

The change perculates into the void comparison, and since it fixes
the order, it removes the current FutureWarning there as well.

This addresses liberfa/pyerfa#77
and replaces numpygh-15509 (the implementation has changed too much).

Fixes numpygh-15494  (and probably a few more)

Co-authored-by: Allan Haldane <[email protected]>
seberg added a commit to seberg/numpy that referenced this issue May 9, 2022
This PR replaces the old numpygh-15509 implementing proper type promotion
for structured voids.  It further fixes the casting safety to consider
casts with equivalent field number and matching order as "safe"
and if the names, titles, and offsets match as "equiv".

The change perculates into the void comparison, and since it fixes
the order, it removes the current FutureWarning there as well.

This addresses liberfa/pyerfa#77
and replaces numpygh-15509 (the implementation has changed too much).

Fixes numpygh-15494  (and probably a few more)

Co-authored-by: Allan Haldane <[email protected]>
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

No branches or pull requests

2 participants