Now more carefully nulling out null inputs in epoch_prop.

[msdemlei]

This is so we do not invent values for pm, parallax, or rv. We are actually a bit over-cautious and invalidate both PMs even if only one is missing. This is because PMs mix, and hence there are traces of the invented 0 in the other component of the PM. Similarly, when the parallax is missing, the RV would be heavily contaminated and hence is nulled out.

Sorry about the whitespace diffs introduced by killing trailing whitespace in sql/epochprop.sql; if they are too confusing, I'll take them into a commit of their own.

See also https://github.com/ivoa-std/udf-catalogue/issues/19, which is about a thin wrapper around epoch_prop.

[esabol]

Failed tests on PostgreSQL 10 and 11. Compared values are accurate to about 1.0e-8, but then the rest of the digits are different. Any ideas?

[msdemlei]

On Thu, May 16, 2024 at 02:14:49PM -0700, Ed Sabol wrote:

Failed tests on PostgreSQL 10 and 11. Compared values are accurate to about 1.0e-8, but then the rest of the digits are different. Any ideas?

The background is that after pg 11, postgres does shortest-precise formatting of floats, before that, extra_float_digits really mattered. I had forgotten that 10 and 11 are still tested, and hence removed the setting from the test case. When I noticed that's trouble, I restored extra_float_digits=2.

Why it's still failing is a bit of a mystery. I'll only get to look into it next monday or so. If someone beat me to it, that's of course great.

[msdemlei]

On Fri, May 17, 2024 at 01:01:21PM +0200, msdemlei wrote:

The background is that after pg 11, postgres does shortest-precise formatting of floats, before that, extra_float_digits really

Ummmm. Perhaps not. I'd appreciate another eye on this after all. What's wrong with pg<12 is something else. Cf. https://github.com/msdemlei/pgsphere/actions/runs/9112032964/job/25050439651#step:7:61, where it says in the diff:

           RADIANS(-801.551/3.6e6), RADIANS(10362/3.6e6), -110,
           20) AS tp;

! tp ! ------------------------------------------- ! (4.702747926583129 , 0.08291945093459933) (1 row)

So... I don't even know what the bang is supposed to mean as a diff character. diff (1) doesn't say.

Can anyone help out?

[esabol]

@msdemlei wrote:

I don't even know what the bang is supposed to mean as a diff character. diff (1) doesn't say. Can anyone help out?

https://unix.stackexchange.com/questions/325657/what-does-an-exclamation-mark-mean-in-diff-output

[df7cb]

Luckily, you can soonish forget again what "context" diffs are, pg_regress was switched to "normal" unified diffs in PG12.

[esabol]

So are the numbers different under PG 10 and 11? If so, why are they different?

[msdemlei]

On Wed, May 22, 2024 at 12:49:32PM -0700, Ed Sabol wrote:

retvals[4] = Float8GetDatum(output.pm[1]); retvals[5] = Float8GetDatum(output.rv);

Nitpick: Added unnecessary whitespace here.

Hm... I have to admit tabs in empty lines are inconsistent in this source; there are empty lines that have them and empty lines that don't. I give you this inconsistency isn't nice, but for simplicity I'd say we should change to no-whitespace empty lines, because that's simpler for the computer to do.

I've just pushed that, and I think this PR would be ready for review (and hopefully merging:-). Any takers? Thanks!

[esabol]

We don't use the epochprop stuff and I'm not really clear on the reason for these changes or what they mean to change, so I'll defer to @vitcpp or @df7cb for a review. I don't have any more nitpicky formatting things, and all the tests pass, so it's looks OK to me.

[vitcpp]

1. A little bit off-topic. I've found unused definition s_cpoint/CPoint in epochprop.h. May be, remove it?

2. Struct s_phasevec has wrong comment for parallax_valid. It describes that parallax_valid = 1 for NULL which is not correct.

[vitcpp]

If to set pm_long, pm_lat to NULL, 1.0 accordingly, pm_lat value will be used in calculations as a non-null value. Not sure, how it can affect the result. I would propose to rewrite this code to make the logic more clear.

Original code:

Proposed code:

if (PG_ARGISNULL(2) || PG_ARGISNULL(3)) { ... } else { ... }

[vitcpp]

Just some thoughts, if parallax is NULL, may be to set RV in output like in input?

[msdemlei]

On Fri, May 24, 2024 at 05:22:22AM -0700, Vitaly wrote:

if (PG_ARGISNULL(2) || PG_ARGISNULL(3)) { ... } else { ... }

Uh... that was actually a bug. Good catch; if no pmra was given but a pmdec, the pmdec would be accepted. Nulls... ah well.

Thanks for the review! Your points should be addressed in 3366ce3, including the input RV on output when the parallax is NULL, which was already subject of a063d3ed.

[msdemlei]

On Fri, May 24, 2024 at 08:52:13AM -0700, Ed Sabol wrote:

@esabol commented on this pull request. Meant to ask before: Is the "-###.###" intentional? If not, it seems to me you should change '999D999' on line 25 to increase the space for the digits to get a real answer.

Ouch. Thanks for paying attention. This is another reason to pull through the input RV: A parallax of 0, and so the -###.### was really a sign that something was wrong: I shouldn't have made RV copying conditional on NULL in the parallax input.

In commit 046a50c, I am now using parallax_valid again to notice when I should not use the RV coming out of epoch propagation. And this gives now a sane result here, too.

[vitcpp]

@msdemlei Could you please rebase the PR? Thank you in advance.

[msdemlei]

On Wed, May 29, 2024 at 08:17:08AM -0700, Vitaly wrote:

@msdemlei Could you please rebase the PR? Thank you in advance.

rebased and force-pushed. Thanks for the review!

[vitcpp]

@esabol Are you ok with the PR?

[esabol]

@vitcpp asked:

@esabol Are you ok with the PR?

Yes, it looks good to me!

[vitcpp]

@msdemlei I'm ready to merge this PR. I would like to propose to squash these commits into the single one. Let me know please if you want to do it. Otherwise, I will do it using github's Squash and Merge.

[msdemlei]

On Mon, Jun 03, 2024 at 01:45:24AM -0700, Vitaly wrote:

@msdemlei I'm ready to merge this PR. I would like to propose to squash these commits into the single one. Let me know please if you want to do it. Otherwise, I will do it using github's Squash and Merge.

There's a whitespace-only commit in there, and I tend to prefer to have them separate, but it's not major, so I'm squashing. Thanks!