Solving issue with wcs.wcs.cunit equality #8480

geekypathak21 · 2019-03-06T23:18:53Z

EDIT: Fix #8415

Please Help me out in this after a lot reading from here and there I was able to apply this because Embedded c with python is a new topic for me. So, please forgive any mistakes done by me .
__eq__ function was not working so I used richcompare function

codecov · 2019-03-06T23:38:44Z

Codecov Report

Merging #8480 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #8480      +/-   ##
==========================================
- Coverage   86.79%   86.76%   -0.03%     
==========================================
  Files         386      386              
  Lines       58098    58117      +19     
  Branches     1060     1065       +5     
==========================================
  Hits        50427    50427              
- Misses       7056     7075      +19     
  Partials      615      615

Impacted Files	Coverage Δ
astropy/wcs/src/unit_list_proxy.c	`46.89% <0%> (-7.08%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69e2fab...4e080b6. Read the comment docs.

bsipocz · 2019-03-07T01:34:41Z

This will need testing, as a minimal one e.g. the example from the issue would do.

codecov · 2019-03-08T17:53:30Z

Codecov Report

Merging #8480 into master will decrease coverage by <.01%.
The diff coverage is 62.5%.

@@            Coverage Diff             @@
##           master    #8480      +/-   ##
==========================================
- Coverage   86.82%   86.81%   -0.01%     
==========================================
  Files         386      386              
  Lines       58197    58213      +16     
  Branches     1060     1064       +4     
==========================================
+ Hits        50530    50540      +10     
- Misses       7052     7054       +2     
- Partials      615      619       +4

Impacted Files	Coverage Δ
astropy/wcs/src/unit_list_proxy.c	`54.92% <62.5%> (+0.96%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fda61c9...9a91f69. Read the comment docs.

geekypathak21 · 2019-03-08T17:57:47Z

I added test for wcs.cunit @bsipocz Thanks for helping

astropy/wcs/tests/test_wcs.py

geekypathak21 · 2019-03-09T17:52:37Z

Another wcs is added in test_wcs.py thanks for suggestion @astrofrog

MSeifert04 · 2019-03-09T18:04:32Z

Thanks for the contribution. I found it quite hard to read the code because it doesn't follow the coding conventions in the rest of the file (nor PEP 7). Could you please re-format the code (especially with regards to whitespaces, indentation, and blank lines) to follow the style of the rest of the file?

MSeifert04 · 2019-03-09T18:08:40Z

astropy/wcs/src/unit_list_proxy.c

+        Py_RETURN_FALSE;
+      }
+    } else {
+      return NULL;


Returning NULL without an exception will probably result in a SystemError - or is that handled differently for richcmp? Did you check this path?

By the way I would prefer it to immediately fail in case an exception should be raised. That way one could also remove the status variable completely.

MSeifert04 · 2019-03-09T18:09:41Z

astropy/wcs/src/unit_list_proxy.c

+    if (self->size!=other->size||self->unit_class!=other->unit_class||self->array!=other->array)
+       {status = 0;
+        equal = 0;}
+    else if (self == 0x0 || other == 0x0){


0x0? Do you mean NULL?

MSeifert04 · 2019-03-09T18:11:39Z

astropy/wcs/src/unit_list_proxy.c

+      PyObject_TypeCheck(other, &PyUnitListProxyType)) {
+
+
+    if (self->size!=other->size||self->unit_class!=other->unit_class||self->array!=other->array)


I don't think this is correct to use pointer equality for unit_class or array. Are these singletons?

I felt so that they are singletons. I will check it again

MSeifert04 · 2019-03-09T18:15:16Z

Thank you for the pull request. I have some questions about the changes but I'm not really familiar with the WCS C-internals so forgive me if these may sound stupid. 😄

Btw: a first-time contributor that dares to go into Python C Extensions - I'm genuinely impressed! 👍 Let me know if you need any help or my comments don't make sense to you.

MSeifert04 · 2019-03-09T18:21:15Z

astropy/wcs/src/unit_list_proxy.c

@@ -4,6 +4,8 @@
 */

 #define NO_IMPORT_ARRAY
+#ifndef ASTROPY_WCS_API_H
+#define ASTROPY_WCS_API_H


Not sure if this is correct. As far as I see this constant should be defined only in astropy_wcs_api.h

Yes I am removing this. It is not correct

MSeifert04 · 2019-03-09T19:40:26Z

The flake check did detect some problems in the Python file too:

 ------ file ----------------:line:col: problem
astropy/wcs/tests/test_wcs.py:1189:23: W291 trailing whitespace
astropy/wcs/tests/test_wcs.py:1192:1: W293 blank line contains whitespace
astropy/wcs/tests/test_wcs.py:1193:39: W291 trailing whitespace
astropy/wcs/tests/test_wcs.py:1200:1: W391 blank line at end of file
astropy/wcs/tests/test_wcs.py:1200:1: W293 blank line contains whitespace

Could you fix them so the CI can succeed?

geekypathak21 · 2019-03-10T21:42:56Z

@MSeifert04 I need help I can't able to find any method to convert PyObject to C type for comparison .
Will value_fixer() work for it or not

MSeifert04 · 2019-03-11T07:09:36Z

@himanshupathak21061998 I'm not sure what you mean with value_fixer. Could you provide more context where you want to convert PyObject to C?

geekypathak21 · 2019-03-13T18:20:01Z

@MSeifert04 Sorry for late reply but I was not able to find any good way to compare unit_class which is a pyobject. I used RichCompareBool function to compare it thanks for your help.

MSeifert04

Thanks for the update. This looks really good.

The indentation still seems to be a bit off though and I have some comments (again).

MSeifert04 · 2019-03-13T19:23:58Z

astropy/wcs/src/unit_list_proxy.c

@@ -16,6 +16,7 @@

 static PyTypeObject PyUnitListProxyType;

+


I don't think the additional blank line is needed

MSeifert04 · 2019-03-13T19:26:03Z

astropy/wcs/src/unit_list_proxy.c

@@ -186,6 +187,59 @@ PyUnitListProxy_getitem(
  return result;
 }

+static PyObject *
+PyUnitListProxy_richcmp(
+    PyUnitListProxy *a,


By assuming PyUnitListProxy here instead of PyObject (see richcmpfunc) we might get segfaults if you compare them to non PyUnitListProxy. (could you add a test case for a different class?)

astropy/wcs/src/unit_list_proxy.c

astropy/wcs/tests/test_wcs.py

astropy/wcs/src/unit_list_proxy.c

MSeifert04 · 2019-03-13T19:43:05Z

@himanshupathak21061998 Thanks for the update, this looks really good 👍

I was not able to find any good way to compare unit_class which is a pyobject. I used RichCompareBool function to compare it thanks for your help.

If unit_class implements rich-compare then this should be fine. I suspected that you meant this, but wasn't sure.

Sorry for late reply

No problem. By the way: I'm away the next few days and probably cannot reply until next Tuesday. 😅

MSeifert04 · 2019-03-13T19:51:45Z

@pllim Sorry for pinging you - but are the test failures expected? They seem unrelated but I haven't been really active in the past so I don't know 😓

pllim · 2019-03-14T17:06:51Z

@MSeifert04 , I just saw these errors on another PR and also on cron (https://travis-ci.org/astropy/astropy/jobs/506241000). Will have to investigate but I don't think they are related to this PR.

UPDATE: Failures are unrelated and caused by #8498

MSeifert04 · 2019-03-14T17:20:30Z

@pllim Thank you for the investigation. 👍

I was on the mobile and did not want to look at the travis logs (because they tend to load for a while) and forgot when I came home. 😅

geekypathak21 · 2019-03-17T09:05:42Z

Not sure for the cause of error https://ci.appveyor.com/project/Astropy/astropy/builds/23131678

MSeifert04 · 2019-03-19T19:02:54Z

@himanshupathak21061998 Do you let me know when the comments are addressed so I can do a final review (although @nden probably needs to do the final review/approval).

geekypathak21 · 2019-03-19T19:22:29Z

@MSeifert04 I am struggling with this

By assuming PyUnitListProxy here instead of PyObject (see richcmpfunc) we might get segfaults if you compare them to non PyUnitListProxy. (could you add a test case for a different class?)

Because when I accept Pyobject and compare Objects inside PyUnitListProxy struct. It always returning true can you please suggest a way to tackle this

MSeifert04 · 2019-03-19T19:54:29Z

The richcmpfunc expects (PyObject *a, PyObject *b, int op) so the function arguments should be equal to these:

static PyObject *
PyUnitListProxy_richcmp(
    PyObject *a,
    PyObject *b,
    int op) {

Because we don't want to compare them to anything else except other PyUnitListProxy you immediately check if they are:

PyUnitListProxy *lhs, *rhs;
/* Normally these shouldn't be NULL, ever */
assert(a != NULL && b != NULL);

/* At the moment we only support comparisons of PyUnitListProxy with other instances of the same class */
if (!PyObject_TypeCheck(a, &PyUnitListProxyType) 
        || !PyObject_TypeCheck(b, &PyUnitListProxyType) {
    Py_RETURN_NOTIMPLEMENTED;
}
/* Only equality and non-equality are supported */
if (op != Py_EQ && op != Py_NE)) {
    Py_RETURN_NOTIMPLEMENTED;
}
lhs = (PyUnitListProxy *)a;
rhs = (PyUnitListProxy *)b;

Then continue with lhs and rhs.

For example I would then immediately check if PyObject_RichCompareBool works:

int equal = PyObject_RichCompareBool(lhs->unit_class, rhs->unit_class, Py_EQ);
if (equal == -1) {
    return NULL;  // Exception will be set because the rich-compare failed
}

and so on.

I tend to prefer failing immediately because it avoids deep (and complex) nestings later.

astropy/wcs/tests/test_wcs.py

geekypathak21 · 2019-03-26T07:43:41Z

@MSeifert04 thanks for helping I added all things as suggested by you. Sorry for late updates

astropy/wcs/tests/test_wcs.py

geekypathak21 · 2019-03-27T07:26:22Z

@MSeifert04 Now should I squash commits. If everything is done

MSeifert04 · 2019-03-27T07:38:36Z

@himanshupathak21061998 I'll do another review later today. Thanks for the update

geekypathak21 · 2019-03-28T07:11:11Z

@MSeifert04 Please review changes

bsipocz · 2019-03-29T01:48:28Z

@himanshupathak2106199 - please be patient, all core maintainers have different day jobs than maintaining astropy, so having to wait a couple of days (or weeks in cases) for a review is part of the game.

CHANGES.rst

MSeifert04 · 2019-04-10T06:02:20Z

Sorry for the long delay, I've been a bit busy. This looks good to me (from a technical viewpoint), the changelog could be clarified a bit though.

@astrofrog or @nden what do you think?

geekypathak21 · 2019-04-10T09:38:53Z

@MSeifert04 Thanks for reviewing and helping me I have changed the Changelog.rst

nden · 2019-04-17T12:01:59Z

@himanshupathak21061998 This looks good! Thanks for the contribution and sorry for the late review.
@MSeifert04 Thanks for the reviews.

I am not sure what the codecov report means. It looks like it points to files in samp not in wcs but I may be misreading it. So I think this is ready to merge.

@himanshupathak21061998 can you squash the commits?

geekypathak21 · 2019-04-17T19:01:39Z

@nden Thanks for reviewing I have done with squashing the commits as suggested

geekypathak21 · 2019-04-17T19:26:16Z

Again Codecov failure😁

nden · 2019-04-17T21:23:02Z

@himanshupathak21061998 Could you add tests for

an error is raised by w1.wcs.cunit == [1, 2, 3]
an error is raised by w1.wcs.cunit == w2.wcs.cunit

I hope this will solve the coverage issue.

geekypathak21 · 2019-04-17T22:39:38Z

astropy/wcs/tests/test_wcs.py

+    # Inequality checking of cunit with a list of literals
+    assert not w1.wcs.cunit == [1, 2, 3]
+    # Inequality checking with some characters
+    assert w1.wcs.cunit != ['a', 'b', 'c']


I added this test because of !=

geekypathak21 · 2019-04-17T23:02:13Z

astropy/wcs/src/unit_list_proxy.c

+  lhs = (PyUnitListProxy *)a;
+  rhs = (PyUnitListProxy *)b;
+  int equal = PyObject_RichCompareBool(lhs->unit_class, rhs->unit_class, Py_EQ);
+  if (equal == -1) {


Coverage report failure is because of this case as PyObject_RichCompareBool is not returning -1 in any of the case present in the test. For this I think, it needs direct testing of PyUnitListProxy which will be more cumbersome. So right now we can merge it

geekypathak21 · 2019-04-17T23:03:31Z

@nden Can you please review it again Thanks for helping

nden

Thanks @himanshupathak21061998 ! I am going to merge this as it is.

geekypathak21 · 2019-04-19T19:34:08Z

Thanks for merging

pllim added wcs Bug labels Mar 7, 2019

pllim requested review from astrofrog and nden March 7, 2019 00:20

astropy deleted a comment from codecov bot Mar 7, 2019

astrofrog reviewed Mar 8, 2019

View reviewed changes

astropy/wcs/tests/test_wcs.py Show resolved Hide resolved

MSeifert04 reviewed Mar 9, 2019

View reviewed changes

MSeifert04 reviewed Mar 13, 2019

View reviewed changes

MSeifert04 reviewed Mar 24, 2019

View reviewed changes

astropy/wcs/tests/test_wcs.py Outdated Show resolved Hide resolved

geekypathak21 force-pushed the WCSequality branch from 391223b to ad51a36 Compare March 26, 2019 07:18

MSeifert04 reviewed Mar 26, 2019

View reviewed changes

astropy/wcs/tests/test_wcs.py Outdated Show resolved Hide resolved

MSeifert04 reviewed Apr 10, 2019

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

geekypathak21 force-pushed the WCSequality branch from 98ce78d to b80fce8 Compare April 10, 2019 09:34

geekypathak21 force-pushed the WCSequality branch from b80fce8 to 2d2501b Compare April 17, 2019 18:55

geekypathak21 added 2 commits April 18, 2019 03:02

Solving issue with wcs.wcs.cunit equality

ccc2bfc

Test for wcs.wcs.cunit is added

a580a0d

geekypathak21 force-pushed the WCSequality branch from 2d2501b to 9478ea0 Compare April 17, 2019 21:51

Changes in inequality test check

9a91f69

geekypathak21 force-pushed the WCSequality branch from 9478ea0 to 9a91f69 Compare April 17, 2019 22:31

geekypathak21 commented Apr 17, 2019

View reviewed changes

nden approved these changes Apr 19, 2019

View reviewed changes

nden merged commit d1f2fd5 into astropy:master Apr 19, 2019

geekypathak21 deleted the WCSequality branch July 31, 2019 20:42

MSeifert04 mentioned this pull request Aug 21, 2019

Fixed WCS.wcs.cunit equality #9154

Merged

		PyObject_TypeCheck(other, &PyUnitListProxyType)) {


		if (self->size!=other->size\|\|self->unit_class!=other->unit_class\|\|self->array!=other->array)

Solving issue with wcs.wcs.cunit equality #8480

Solving issue with wcs.wcs.cunit equality #8480

Conversation

geekypathak21 commented Mar 6, 2019 • edited by pllim

codecov bot commented Mar 6, 2019

Codecov Report

bsipocz commented Mar 7, 2019

codecov bot commented Mar 8, 2019 • edited

Codecov Report

geekypathak21 commented Mar 8, 2019

geekypathak21 commented Mar 9, 2019

MSeifert04 commented Mar 9, 2019 • edited

MSeifert04 Mar 9, 2019

Choose a reason for hiding this comment

MSeifert04 Mar 9, 2019 • edited

Choose a reason for hiding this comment

MSeifert04 Mar 9, 2019

Choose a reason for hiding this comment

geekypathak21 Mar 9, 2019

Choose a reason for hiding this comment

MSeifert04 commented Mar 9, 2019 • edited

MSeifert04 Mar 9, 2019

Choose a reason for hiding this comment

geekypathak21 Mar 9, 2019 • edited

Choose a reason for hiding this comment

MSeifert04 commented Mar 9, 2019 • edited

geekypathak21 commented Mar 10, 2019

MSeifert04 commented Mar 11, 2019

geekypathak21 commented Mar 13, 2019

MSeifert04 left a comment

Choose a reason for hiding this comment

MSeifert04 Mar 13, 2019

Choose a reason for hiding this comment

MSeifert04 Mar 13, 2019

Choose a reason for hiding this comment

MSeifert04 commented Mar 13, 2019

MSeifert04 commented Mar 13, 2019 • edited

pllim commented Mar 14, 2019 • edited

MSeifert04 commented Mar 14, 2019

geekypathak21 commented Mar 17, 2019

MSeifert04 commented Mar 19, 2019

geekypathak21 commented Mar 19, 2019 • edited

MSeifert04 commented Mar 19, 2019 • edited

geekypathak21 commented Mar 26, 2019

geekypathak21 commented Mar 27, 2019

MSeifert04 commented Mar 27, 2019

geekypathak21 commented Mar 28, 2019 • edited

bsipocz commented Mar 29, 2019 • edited

MSeifert04 commented Apr 10, 2019

geekypathak21 commented Apr 10, 2019

nden commented Apr 17, 2019

geekypathak21 commented Apr 17, 2019 • edited

geekypathak21 commented Apr 17, 2019

nden commented Apr 17, 2019

geekypathak21 Apr 17, 2019

Choose a reason for hiding this comment

geekypathak21 Apr 17, 2019 • edited

Choose a reason for hiding this comment

geekypathak21 commented Apr 17, 2019

nden left a comment

Choose a reason for hiding this comment

geekypathak21 commented Apr 19, 2019

geekypathak21 commented Mar 6, 2019 •

edited by pllim

codecov bot commented Mar 8, 2019 •

edited

MSeifert04 commented Mar 9, 2019 •

edited

MSeifert04 Mar 9, 2019 •

edited

MSeifert04 commented Mar 9, 2019 •

edited

geekypathak21 Mar 9, 2019 •

edited

MSeifert04 commented Mar 9, 2019 •

edited

MSeifert04 commented Mar 13, 2019 •

edited

pllim commented Mar 14, 2019 •

edited

geekypathak21 commented Mar 19, 2019 •

edited

MSeifert04 commented Mar 19, 2019 •

edited

geekypathak21 commented Mar 28, 2019 •

edited

bsipocz commented Mar 29, 2019 •

edited

geekypathak21 commented Apr 17, 2019 •

edited

geekypathak21 Apr 17, 2019 •

edited