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

Now more carefully nulling out null inputs in epoch_prop. #124

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

msdemlei
Copy link
Contributor

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 ivoa-std/udf-catalogue#19, which is about a thin wrapper around epoch_prop.

@msdemlei msdemlei force-pushed the epoch-prop-nulling branch from 3bf410a to 8820ca0 Compare May 16, 2024 12:13
@esabol
Copy link
Contributor

esabol commented May 16, 2024

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
Copy link
Contributor Author

msdemlei commented May 17, 2024 via email

@msdemlei
Copy link
Contributor Author

msdemlei commented May 21, 2024 via email

@esabol
Copy link
Contributor

esabol commented May 21, 2024

@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
Copy link
Contributor

df7cb commented May 21, 2024

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

@esabol
Copy link
Contributor

esabol commented May 21, 2024

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

src/epochprop.c Outdated
/* change to internal units: rad, rad/yr, mas, and km/s */
retvals[0] = Float8GetDatum(output.pos.lng);
retvals[1] = Float8GetDatum(output.pos.lat);
retvals[2] = Float8GetDatum(output.parallax);
retvals[3] = Float8GetDatum(output.pm[0]);
retvals[4] = Float8GetDatum(output.pm[1]);
retvals[5] = Float8GetDatum(output.rv);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Added unnecessary whitespace here.

@msdemlei
Copy link
Contributor Author

msdemlei commented May 23, 2024 via email

@esabol
Copy link
Contributor

esabol commented May 23, 2024

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
Copy link
Contributor

vitcpp commented May 24, 2024

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

image

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

@vitcpp
Copy link
Contributor

vitcpp commented May 24, 2024

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:

image

Proposed code:

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

@vitcpp
Copy link
Contributor

vitcpp commented May 24, 2024

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

@msdemlei
Copy link
Contributor Author

msdemlei commented May 24, 2024 via email

269.4744079540 | 4.4055337210 | | -801.210 | 10361.762 |
to_char | to_char | to_char | to_char | to_char | to_char
-----------------+-----------------+----------+----------+------------+----------
269.4744079540 | 4.4055337210 | .000 | -801.210 | 10361.762 | -###.###
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@msdemlei
Copy link
Contributor Author

msdemlei commented May 27, 2024 via email

@vitcpp
Copy link
Contributor

vitcpp commented May 29, 2024

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

@msdemlei msdemlei force-pushed the epoch-prop-nulling branch from 046a50c to b81042b Compare May 31, 2024 08:09
@msdemlei
Copy link
Contributor Author

msdemlei commented May 31, 2024 via email

@vitcpp
Copy link
Contributor

vitcpp commented May 31, 2024

@esabol Are you ok with the PR?

@esabol
Copy link
Contributor

esabol commented May 31, 2024

@vitcpp asked:

@esabol Are you ok with the PR?

Yes, it looks good to me!

@vitcpp
Copy link
Contributor

vitcpp commented Jun 3, 2024

@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.

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 or bad, the RV would be heavily contaminated;
in these cases, we copy through the original RV, as it may still be useful
and certainly will not be grossly wrong.

(sorry about a few whitespace diffs introduced by killing trailing whitespace
in sql/epochprop.sql and src/epochprop.c)
@msdemlei msdemlei force-pushed the epoch-prop-nulling branch from b81042b to 70b821c Compare June 5, 2024 08:12
@msdemlei
Copy link
Contributor Author

msdemlei commented Jun 5, 2024 via email

@vitcpp vitcpp merged commit e1dee24 into postgrespro:master Jun 10, 2024
14 checks passed
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

Successfully merging this pull request may close these issues.

4 participants