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

[FROZEN] Re-add ivo_epoch_prop #20

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

msdemlei
Copy link
Collaborator

This is an attempt to resolve bug #19.

(also, a minor editorial addition to the sexagesimal translation functions)

(also, a minor editorial addition to the sexagesimal translation functions)
Comment on lines 406 to 407
other component must be NULL-ed out on output, too. Similarly, when a
NULL parallax is passed in, the RV must be NULL-ed out on output, too.
Copy link
Member

Choose a reason for hiding this comment

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

Why does a NULL input parallax require a NULL output rv? If the rv is OK on input but you don't happen to have a parallax for it, the same rv should be a reasonable value for the propagated version. Am I missing something?

@msdemlei
Copy link
Collaborator Author

msdemlei commented May 16, 2024 via email

@msdemlei msdemlei mentioned this pull request May 16, 2024
@msdemlei msdemlei changed the title Improving the rules for NULL-ing out output values in epoch_prop. Re-add ivo_epoch_prop May 16, 2024
@msdemlei
Copy link
Collaborator Author

msdemlei commented May 16, 2024

ivo_epoch_prop is gone from master again, as the ESAVO implementation differs from what is described here. This PR should remain open until a consensus about the signature is reached (or closed if we decide ivo_epoch_prop is not worth it at all).

@mbtaylor
Copy link
Member

I prefer to restore the non-transformed RV in case of NULL parallax.

The use case I'm thinking of, and my guess is it's the dominant one for users of this UDF, is that they want the best available estimate of the astrometric quantities for an epoch in the future/past, given available information. If available information is good enough to improve the values at the current epoch, then do it. If not, fall back to the current epoch values. And don't fabricate a definite value for a quantity at the new epoch if it didn't have one at the current epoch.

So applied to position that means: if there's a PM, apply that to get a new position, and if not, use the current position. Applied to RV it means: if there's a parallax, apply it to get a new RV, and if not, use the current RV.

I think(?) that your proposal to NULL output RV in case of NULL input parallax would be treating the RV/parallax case differently than the position/PM case. For consistency with your proposal you'd want to NULL output position if input PM is missing. That would be reasonable behaviour, but as I commented in #19 I don't think it's what most people are after here.

@msdemlei
Copy link
Collaborator Author

msdemlei commented May 22, 2024 via email

@msdemlei msdemlei changed the title Re-add ivo_epoch_prop [FROZEN] Re-add ivo_epoch_prop May 22, 2024
mbtaylor
mbtaylor previously approved these changes May 22, 2024
Copy link
Member

@mbtaylor mbtaylor left a comment

Choose a reason for hiding this comment

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

Thanks. Checking my code against the examples here threw up some inconsistencies with my NULL processing as well, so thanks for providing all those too.

@jontxu
Copy link
Collaborator

jontxu commented May 22, 2024

I see Mark has approved the changes... are we waiting for a PR in pgsphere before this one can be merged?

@msdemlei
Copy link
Collaborator Author

msdemlei commented May 23, 2024 via email

@mbtaylor
Copy link
Member

I have brought this to ESAC's attention (JIRA issue C9GACS-825, only visible to DPAC members).

@mbtaylor
Copy link
Member

I came across two very minor issues with the examples in the text while using it to write unit tests for my code:

  1. the use of NaN and NULL is not quite consistent: blank values in returned arrays are mostly represented as NULL but there is a NaN parallax in the return value for the third example. I don't really care whether output blank values are written as NULL or NaN but it should be consistent.
  2. The unchanged ra value is propagated from 7.606083572 to 7.606083572000001 in the fourth example. Given precision expectations this isn't wrong, but it's slightly more confusing than it could be, so clarity might be improved by chopping off that trailing 000001.

@msdemlei
Copy link
Collaborator Author

msdemlei commented May 27, 2024 via email

Comment on lines +434 to +438
\example \begin{lstlisting}
ivo_epoch_prop(7.606083572, 11.79044105, 125,
21, NULL, NULL, 2016.0, 1992.25)
\end{lstlisting}
\becomes [7.606083572, 11.79044105, 125.0, NULL, NULL, NULL]
Copy link
Member

Choose a reason for hiding this comment

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

This example is a bit weird - pmra defined but pmdec NULL on input. It's not a very likely case, and the most sensible output is debatable - you could argue that instead of nulling out both PM elements on output it should leave the known pmra intact and return exactly the same output as input. Was it actually intended like this? I wonder if it's supposed to be

ivo_epoch_prop(7.606083572, 11.79044105, 125,  NULL, NULL, 21, 2016.0, 1992.25)

instead - which I think would be a more instructive example.

@msdemlei
Copy link
Collaborator Author

msdemlei commented Jun 5, 2024 via email

@mbtaylor
Copy link
Member

mbtaylor commented Jun 5, 2024

Fair enough, thanks for the clarification.

@gmantele
Copy link
Contributor

gmantele commented Jul 3, 2024

I am no expert here and I completely rely on both of you for this issue.
Is it still supposed to be frozen or could it be merged?

Do you have any news from ESAC guys? If not, should we ping them again?

@mbtaylor
Copy link
Member

mbtaylor commented Jul 3, 2024

I believe that @msdemlei marked this PR frozen since we only have one implementation of the 8-parameter ivo_epoch_prop_pos UDF under discussion, namely Markus's.

ESAC have said (issue C9GACS-825, restricted access):.

  • We (at ESDC) will create a new ivo_epoch_prop UDF which will differ from esdc_epoch_prop:
  • It will take radial velocity as input and will return radial velocity as output (so the argument will become symmetric);
  • It will adhere to the new agreements reached for the NULL behaviour;
  • I will update this ticket accordingly and will then let it enter a future sprint. Since we are focused on a massive GACS infrastructure migration for the coming half year, this may shift to 2025.

So they do intend to provide an equivalent implementation in GACS, but it may not be soon.
Options for progress:

  • Wait until ESDC implement it before merging
  • Take ESDC's agreement in principle as sufficient for the multiple implementations criterion and merge
  • Persuade somebody else (@jontxu?) to implement it and then merge

@jontxu
Copy link
Collaborator

jontxu commented Jul 3, 2024

Persuade somebody else (@jontxu?) to implement it and then merge

Could be done but... would it be considered a second implementation if it uses the same codebase? As it would also be based on pgsphere.

@msdemlei
Copy link
Collaborator Author

msdemlei commented Jul 3, 2024 via email

@gmantele
Copy link
Contributor

gmantele commented Jul 4, 2024

Thank you to all of you for your answers. We will wait for ESAC then 🙂

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