taskGNU Astronomy Utilities - Tasks: task #16288, Warp: warn users when WCSLIB...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

task #16288: Warp: warn users when WCSLIB version is <7.12

Submitter:  Pedram Ashofteh-Ardakani <pedram>
Submitted:  Wed 07 Dec 2022 08:52:13 PM UTC
   
 
Should Start On:  Wed 07 Dec 2022 12:00:00 AM UTC Should be Finished on:  Wed 07 Dec 2022 12:00:00 AM UTC
Category:  Warp Priority:  5 - Normal
Item Group:  Crash Status:  None
Privacy:  Public Assigned to:  None
Percent Complete:  0% Open/Closed:  Open
Effort:  0.00

Jump to the original submission

Sun 02 Apr 2023 10:20:57 AM UTC, comment #9: 

I believe my fork is ready to be reviewed.

If anyone has any additional points they'd like to add, please let me know!

Otherwise Mohammad can merge it with his approval.

Labib Asari <labeeb7z>
Wed 29 Mar 2023 03:59:12 PM UTC, comment #8: 

Right Labib :) For other readers who might be interested, here is a reference about distribution of negation over a logic statement, which states:

> !(A && B) = (!A || !B)


So when I tried to remove the extra negation, I should have changed the AND operator to an OR operator. But this alone wouldn't fix the "minor version" problem, the full fix is already  stated in comment #6.

Labib, I am looking forward to seeing your changes merged into the master branch after Mohammad's approval, and your name added to the gnuastro manual's developers list :)

Best wishes :)

Pedram Ashofteh-Ardakani <pedram>
Group Member
Wed 29 Mar 2023 12:46:04 PM UTC, comment #7: 

Thanks Pedram, you were right,

!(A and B) is not equal to (!A and !B)

Your suggestion seems correct, I've proceeded with it.

Labib Asari <labeeb7z>
Wed 29 Mar 2023 11:55:30 AM UTC, comment #6: 

Oops, I found out the condition has a logical bug. For example, version 8.10 wouldn't pass the test correctly, since ver[1] < 12 and there's an && in the condition. To make it right, I suggest this alternative:


if( (ver[0]<7) || (ver[0]==7 && ver[1]<12) ) exit(1);


So now, if the major version number is less than 7, WCSLIB is definitely old. But, if it the major version is 7 AND the minor is less than 12, again, it is old. It is considered as recent otherwise.

Did I get it right Labib?

Pedram Ashofteh-Ardakani <pedram>
Group Member
Wed 29 Mar 2023 11:18:35 AM UTC, comment #5: 

On another note, using negations in conditionals makes the logic complex and harder to understand. I'd suggest to replace the conditional from:


if(!(ver[0]>=7 && ver[1]>=12))exit(1);


To the simpler case shown below:


 if(ver[0]<7 && ver[1]<12)exit(1);


Less paranthesis and no more negations '!' :)

Pedram Ashofteh-Ardakani <pedram>
Group Member
Wed 29 Mar 2023 11:02:39 AM UTC, comment #4: 

Nicely done Labib! The message is clear and concise, and the variables and checks are put in good places. Well done. You should go for the PR :) Mohammad will also take a look at it before merging to master and setting this task as complete.

I only have a question about lines 563-565:


int ver[3]={-1,-1,-1};
wcslib_version(ver);
if(!(ver[0]>=7 && ver[1]>=12))exit(1);


Here, we're declaring `ver` of size 3. Does WCS follow the x.x.x versioning pattern? Or is it just there to follow some kind of a  standard that the `wcslib_version` function is following?

Pedram Ashofteh-Ardakani <pedram>
Group Member
Tue 28 Mar 2023 12:39:12 PM UTC, comment #3: 

Hey Pedram, I worked upon this and here's what I could come up with : https://github.com/labeeb-7z/GnuAstro/tree/wcslib-version-warning

The approach I took is
 - detect if wcslib is present
   - if its present, detect its version using wcslib_version function.
   - if it is older than 7.12, set wcslib_version_old to yes, else no

Finally I print some warnings in Configuration Warnings section if wcslib_version_old is yes

I've also tested and built it locally without any errors.

Let me know if this can improved any further, or I should move ahead by sending a pull request.

Labib Asari <labeeb7z>
Mon 27 Mar 2023 05:25:10 PM UTC, comment #2: 

Hello there Labib,

Thank you for showing interest in completing this task.

Indeed, we can check the WCS version inside 'configure.ac'. The OPTIONAL
warnings echo the following message:

> "OPTIONAL warnings/dependencies (for improved functionality):"


So I'd say since having an 'old' WCS does NOT BREAK the entire program for
every use case, this could be categorized under 'improved functionality'.

About getting the version, maybe we could somehow mimic the
"$gsl_version_old" variable in 'configure.ac'. In case the version is lower
than 7.12, a new variable like "$wcs_version_old" could be set to "yes" and
then a warning could be printed under the OPTIONAL warnings section
informing the user that "warp" might crash in some scenarios (as explained
in the gnuastro manual).

Pedram Ashofteh-Ardakani <pedram>
Group Member
Mon 27 Mar 2023 04:43:27 PM UTC, comment #1: 

Hey Pedram, Im looking forward to solve this task.

I think this can be done by detecting the wcslib version while configure script is run.

If the library is found we also detect its version using

> pkg-config --modversion wcslib


and print a warning if its less than 7.12

A question I have in mind is, since wcslib is a mandatory dependency, should this warning be included in a mandatory dependency missing section or the optional dependency section?

Labib Asari <labeeb7z>
Wed 07 Dec 2022 08:52:13 PM UTC, original submission:  

Sepideh and Mohammad have already let Mark Callabreta know that WCSLIB 7.11 fails when calculating specific coordinates. He has fixed it in WCSLIB 7.12, and in the changelog pointed out:

>  - In wcsp2s() and wcss2p(), fixed handling of status returns from
>    linp2x() and linx2p() relating to distortion functions, specifically
>    with respect to setting the stat[] vector.  Reported by Sepideh
>    Eskandarlou (via Mohammad Akhlaghi).


That is why we have put a warning in Warp's documentation about this and encouraged users to install at least WCSLIB 7.12. However, some might simply miss this important warning. Hence, we should check user's WCSLIB version while running warp, and print a warning to prevent bad results.


Pedram Ashofteh-Ardakani <pedram>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by labeeb7z (Posted a comment)
  • -email is unavailable- added by pedram (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    No changes have been made to this item

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code