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

add in altitude support and simplify adding detector function #4884

Merged
merged 3 commits into from
Oct 2, 2024

Conversation

ahnitz
Copy link
Member

@ahnitz ahnitz commented Sep 19, 2024

  • Adds in the ability to set the altitude angles for custom detectors
  • Simplify detector class, use lal only for its detector location information rather than the response value itself. This makes it easier to provide consistent metadata between custom and lal-builtin detectors
  • Simplify the add detector on the earth function (use matrix multiplication operator, make the rotations clearer / more obvious / start rotations from simpler form of the basic arm response)

Todo

  • update unittest to include both use of custom detector and crosscheck response matrix calculation against builtin lal value

@titodalcanton
Copy link
Contributor

I am unlikely to be able to review this carefully before the end of the month…

Copy link
Contributor

@spxiwh spxiwh left a comment

Choose a reason for hiding this comment

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

I'm fine with adding this altitude angle in. I do want to check what this changes now. For e.g. if someone were doing some ongoing analysis, will detector responses change by this? (I think that we will expect CE to have changes, but not others?)

# value gets a similar number of samples as other detectors
# it is used for nothing else
d.yArmMidpoint = self.info['ylength'] / 2.0
d.xArmMidpoint = self.info['xlength'] / 2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this being changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

@spxiwh The lal code doesn't actually use these numbers for much, but it does have them set properly to the right values. This change means that they will be set in the same way as the internal cached detectors in lal in case lal every does use them (for example if someone were to add high-frequency corrections to lalsuite it might matter). Before, I was not bothering to really set them because in the code itself the only thing it used them for was was a guess to a filter length.

vec = np.matmul(rm.T, np.array([-np.cos(angle), np.sin(angle), 0]))
vecs.append(vec)
resps.append(rm @ resp @ rm.T / 2.0)
vecs.append(rm @ np.array([-1, 0, 0]))
Copy link
Contributor

Choose a reason for hiding this comment

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

So there's changes in here. I think to include the "altitude angles" of the two arms. Have you checked/can you show that this is unchanged in the limit that the altitude angles are both 0.

For current instruments (H1,L1,V1,K1), will this make any actual change?

Copy link
Member Author

Choose a reason for hiding this comment

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

@spxiwh That's actually what I added as a unit test. The key thing is that this sets up the response matrix. I numerically compare the response matrix here against the cached ones in lal. There is no change to the response there (except the E* case which has a 10^-4 level change due to the issue I raised in slack).

The altitudes are near zero for the H1/ L1 / V1 comparison, but not exactly so (nor should they be). In fact if you don't account for the altitude the response matrix would also be different at the O(10^-4) level like E* is.

Copy link
Contributor

@spxiwh spxiwh left a comment

Choose a reason for hiding this comment

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

Okay, I'm happy with this then.

@ahnitz ahnitz merged commit fe9b9ee into gwastro:master Oct 2, 2024
30 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.

3 participants