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

Improve docs for QNM sur #29

Open
3 tasks
vijayvarma392 opened this issue Aug 15, 2024 · 3 comments
Open
3 tasks

Improve docs for QNM sur #29

vijayvarma392 opened this issue Aug 15, 2024 · 3 comments
Assignees

Comments

@vijayvarma392
Copy link
Owner

I should have caught this before merging the PR, but was held up with other things.
I noticed a few things that should be improved:

  • The notebook probably needs to be rerun to remove old TODOs:
image
  • (l,m,n,p) need to properly defined. For example, the allowed values of p are probably just (+1, -1). That and what that means should be stated.

  • Also, qnm conventions are different in different literature, so we should either point to the section in the paper where this is made clear, or repeat that here. For example, being an NR person, I might expect z-axis is always the direction of initial Lhat. Some ringdown people use zhat as the direction of final spin. These two are not the same when the remnant spin is negative. So, which one we use should be made clear in the docs.

duetosymmetry added a commit to duetosymmetry/surfinBH that referenced this issue Aug 16, 2024
Also added metadata.json to silence warning from sxs in notebook; and
re-executed notebook to reflect updated docs.
@vijayvarma392
Copy link
Owner Author

I'll just add this here for convenience:
Are qnm and sxs packages requirements for these fits?
If so, please add them here: https://github.com/vijayvarma392/surfinBH/blob/master/setup.py#L24 and update the README:
image

Remove them from the CI: https://github.com/vijayvarma392/surfinBH/blob/master/.github/workflows/test.yml#L44
We want pip to install the dependencies during the test as well.

@duetosymmetry
Copy link
Collaborator

No, they are not deps for getting these fits from surfinBH. But since the example notebook — which is converted with nbconvert when running tests — imports sxs and qnm, running the tests requires sxs and qnm.

@vijayvarma392
Copy link
Owner Author

No, they are not deps for getting these fits from surfinBH. But since the example notebook — which is converted with nbconvert when running tests — imports sxs and qnm, running the tests requires sxs and qnm.

Ah, all good then, thanks!

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

No branches or pull requests

2 participants