cyjoelchen / php-sweph

PHP Extension for Swiss Ephemeris
Other
63 stars 29 forks source link

Two functions throwing errors during make test on MacOS BigSur & PHP8 #66

Closed AstroSign-Dev closed 3 years ago

AstroSign-Dev commented 3 years ago

Two functions throwing errors during make test on MacOS BigSur & PHP8

2 functions are showing FAIL executing make test. These are:

FAIL Basic test [tests/swe_azalt_refrac.phpt] FAIL Basic test [tests/swe_fixstar.phpt]

all other functions are showing PASS, executing make test

aloistr commented 3 years ago

Please report the content of tests/swe_azalt_refrac.out tests/swe_fixstar.out

AstroSign-Dev commented 3 years ago

Both files are attached swe_azalt_refrac.out.txt swe_fixstar.out.txt

kevindecapite commented 3 years ago

Can you post the diff files instead? Either those, or the "out" and "exp" ones for comparison.

Never mind, I can compare your output with the test files and see the differences that way.

For swe_azalt_refrac(), it's a simple rounding issue:

EXP: swe_azalt_rev(2454503.06, SE_HOR2ECL, 12.1, 49, 330, 249.15226738768, 17.317286330931)
OUT: swe_azalt_rev(2454503.06, SE_HOR2ECL, 12.1, 49, 330, 249.15226738767, 17.317286330931)

I will round all doubles to 6 places to correct this as the discrepancy is minuscule and system dependent.

I am still investigating the swe_fixstar() differences as those values are already rounded to 6 places, yet we still find a difference in values:

EXP: float(-0.008465)
OUT: float(-0.008503)

EXP: float(-0.008465)
OUT: float(-0.008503)
AstroSign-Dev commented 3 years ago

Sure, no problem swe_azalt_refrac.diff.txt swe_fixstar.diff.txt swe_azalt_refrac.exp.txt swe_fixstar.exp.txt

aloistr commented 3 years ago

I could now install php 8.0.9 and build successfully php-sweph. I got errors at make In file included from /usr/local/Cellar/php/8.0.9/include/php/Zend/zend.h:357: /usr/local/Cellar/php/8.0.9/include/php/Zend/zend_operators.h:541:10: error: expected '(' after 'asm' asm goto(

which I fixed by editing /usr/local/Cellar/php/8.0.9/include/php/Zend/zend_operators.h changing line 522 to

# define ZEND_USE_ASM_ARITHMETIC 0

Then make succeeds. make test brings up the same errors. The fixstar error can be fixed by using another star instead of Polaris in the test. I discouraged the use of Polaris before!

The swe_azalt error can fixed by limiting the demanded precision in the result.

aloistr commented 3 years ago

The radial velocity of Polaris is not something I would lose sleep over. round to 4 digits after the comma, not to 4 significant digits, here. I.e. float(-0.0085)

kevindecapite commented 3 years ago

Ok, that is good to know. I'll make the rounding changes and PR them.

aloistr commented 3 years ago

Actually, I recommend not using SEFLG_SPEED in the fixstar functions.

aloistr commented 3 years ago

And, for Polaris, I suggest using SEFLG_EQUATORIAL The equatorial position is much more interesting then the ecliptic position which makes very little sense for a star near the pole.

aloistr commented 3 years ago

dropping SEFLG_SPEED will not help, because swe_fixstar_ut() returns speed anyway, even when the flag is not set. Internally speed is needed and always computed swe_fixstar2_ut() is clever enough to remove speed in output if it was not requested.

kevindecapite commented 3 years ago

Ok I just PRd the fixes but perhaps it still will not work as you just indicated.

aloistr commented 3 years ago

I updated sweph.c so that now swe_fixstar() will not return speed if not asked for. I am not sure you want to have this fix pushed in the master branch?

kevindecapite commented 3 years ago

This is a core library change then, correct? If so, I believe that change/fix should go into the SE library, probably released as a new version, and then updated accordingly in this extension. This would be the "proper" way to do it.

I have a call I must take now and will return ASAP but my schedule is quite busy this morning.

aloistr commented 3 years ago

It is in swisseph Github, in branch 'devel' I will put it into master only in next release 2.11 or 2.10.03 It is actually a program bug, there was code with the intention of not returning speed, but a check was in the wrong place. An update will be a small bugfix, not a feature release. It was just never noticed.

kevindecapite commented 3 years ago

@AstroSign-Dev Can you test this branch to see if the tests now pass for you?

https://github.com/cyjoelchen/php-sweph/pull/67

(i.e. checkout branch issues/66 and then build normally.)

If not, we may need @aloistr to make the change to sweph.c which he mentioned above, before we merge into master.

aloistr commented 3 years ago

I fixed swephp.c in branch issues/66 so that it now drops speed in swe_fixstar() and swe_fixstar(ut) if not requested. It does not depend on a fix in the SE library.

kevindecapite commented 3 years ago

Thank you. It's now merged and released as 4.0.1: https://github.com/cyjoelchen/php-sweph/releases/tag/4.0.1

kevindecapite commented 3 years ago

@AstroSign-Dev I will close this issue after you confirm that these tests do indeed pass in your environment.

AstroSign-Dev commented 3 years ago

Tested again with version 4.0.1 and all tests are of status PASS now, I will close the case.