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

Spherical #203

Closed
wants to merge 13 commits into from
Closed

Spherical #203

wants to merge 13 commits into from

Conversation

jvshields
Copy link
Contributor

@jvshields jvshields commented Jul 25, 2024

Somewhat working spherical geometry. Still needs validation/tests

@jvshields jvshields marked this pull request as draft July 25, 2024 19:34
@jvshields jvshields marked this pull request as ready for review August 14, 2024 18:29
@andrewfullard andrewfullard self-requested a review August 19, 2024 13:26
w1[gap_index, nu_index]
* (
(
source[gap_index + 1, nu_index]
Copy link
Contributor

Choose a reason for hiding this comment

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

Some explanatory comments regarding the indices here would be nice. Or an equation reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The equation reference is in the docstring. I think I didn't actually end up changing the raytracing equation other than not attentuating the ray for 0 optical depth. The rest of the equation just looks so different because black decided to format it differently this time for some reason I don't quite understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the note of indices though, do you think nu_index and gap_index aren't descriptive enough? I thought the names would be enough because they keep track of the gap in depth points you're moving past, or the frequency you're working on. But I've also been staring at this equation for months so I might be blind to it being unclear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, must have missed it in the docstring. The black formatting probably doesn't help either. The index names seem okay, though "gap" is not entirely clear to me. So you could either add the above as a descriptive comment, or find an alternative name.

@jvshields
Copy link
Contributor Author

I would just merge #212 instead of this.

@jvshields jvshields closed this Nov 4, 2024
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.

2 participants