Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vitcpp merged 1 commit intopostgrespro:masterfrommsdemlei:epoch-prop-nulling
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 alsoivoa-std/udf-catalogue#19, which is about a thin wrapper around epoch_prop.

@esabol
Copy link
Contributor

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
ContributorAuthor

msdemlei commentedMay 17, 2024 via email

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-preciseformatting of floats, before that, extra_float_digits reallymattered. I had forgotten that 10 and 11 are still tested, and henceremoved the setting from the test case. When I noticed that'strouble, I restored extra_float_digits=2.Why it's still failing is a bit of a mystery. I'll only get to lookinto it next monday or so. If someone beat me to it, that's ofcourse great.

@msdemlei
Copy link
ContributorAuthor

msdemlei commentedMay 21, 2024 via email

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 diffcharacter. diff (1) doesn't say.Can anyone help out?

@esabol
Copy link
Contributor

@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

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

@esabol
Copy link
Contributor

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

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
ContributorAuthor

msdemlei commentedMay 23, 2024 via email

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 thissource; there are empty lines that have them and empty lines thatdon't. I give you this inconsistency isn't nice, but for simplicityI'd say we should change to no-whitespace empty lines, because that'ssimpler 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
Copy link
Contributor

esabol commentedMay 23, 2024•
edited
Loading

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 itlooks OK to me.

@vitcpp
Copy link
Contributor

vitcpp commentedMay 24, 2024•
edited
Loading

  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.
esabol reacted with thumbs up emoji

@vitcpp
Copy link
Contributor

vitcpp commentedMay 24, 2024•
edited
Loading

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

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

@msdemlei
Copy link
ContributorAuthor

msdemlei commentedMay 24, 2024 via email

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 buta pmdec, the pmdec would be accepted. Nulls... ah well.Thanks for the review! Your points should be addressed in3366ce3,including the input RV on output when the parallax is NULL, which wasalready subject ofa063d3e.
vitcpp reacted with thumbs up emoji

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
ContributorAuthor

msdemlei commentedMay 27, 2024 via email

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 pullthrough the input RV: A parallax of 0, and so the -###.### was reallya sign that something was wrong: I shouldn't have made RV copyingconditional on NULL in the parallax input.In commit046a50c, I am now using parallax_valid again to notice whenI should not use the RV coming out of epoch propagation. And thisgives now a sane result here, too.
esabol and vitcpp reacted with thumbs up emoji

@vitcpp
Copy link
Contributor

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

@msdemlei
Copy link
ContributorAuthor

msdemlei commentedMay 31, 2024 via email

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 reacted with thumbs up emoji

@vitcpp
Copy link
Contributor

@esabol Are you ok with the PR?

@esabol
Copy link
Contributor

@vitcpp asked:

@esabol Are you ok with the PR?

Yes, it looks good to me!

@vitcpp
Copy link
Contributor

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

esabol reacted with thumbs up emoji

This is so we do not invent values for pm, parallax, or rv.  Weare actually a bit over-cautious and invalidate both PMs even ifonly one is missing.  This is because PMs mix, and hence there are tracesof 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 usefuland certainly will not be grossly wrong.(sorry about a few whitespace diffs introduced by killing trailing whitespacein sql/epochprop.sql and src/epochprop.c)
@msdemlei
Copy link
ContributorAuthor

msdemlei commentedJun 5, 2024 via email

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 tohave them separate, but it's not major, so I'm squashing. Thanks!
vitcpp reacted with thumbs up emoji

@vitcppvitcpp merged commite1dee24 intopostgrespro:masterJun 10, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@vitcppvitcppvitcpp approved these changes

+1 more reviewer

@esabolesabolesabol approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@msdemlei@esabol@df7cb@vitcpp
[8]ページ先頭
©2009-2025 Movatter.jp