-
Notifications
You must be signed in to change notification settings - Fork 95
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
Create cosmic_string #270
base: main
Are you sure you want to change the base?
Create cosmic_string #270
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR of this interesting model!
I left some tasks as TODO for the implementation to follow the conventions of the LensModel module, provide array-based support and make it cosmology independent.
In parallel, I request you to write test functions for each individual definition. lenstronomy is effectively mirrored in the /test/ folder and you should create:
- a file /test/test_LensModel/test_Profiles/test_cosmic_string.py
- add unit tests for each definition and execute tasks where you know the outcome. Also test with arrays and single input values.
- add a function to test that the numerical differentials result in the same (or very similar) outcome as the analytic relations between function, derivatives and hessian here: https://github.com/sibirrer/lenstronomy/blob/main/test/test_LensModel/test_numeric_lens_differentials.py
Let me know if you have questions!
Simon
return alpha_x, alpha_y | ||
|
||
def hessian(self, x, y): | ||
f_xx, f_xy, f_yx, f_yy = 0, 0, 0, 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the event of x and y being arrays (which must be supported), the returns should also be arrays. E.g. np.zeros_like(x)
|
||
s = 2 * alpha_hat(mu) * (self.ds - self.dd) + alpha_reduced(mu) | ||
|
||
if x > s or x < -1 * s or x == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, these code lines only support single values for 's', and thus do not support an array-based execution with multiple entries. Here is an example for conditions on arrays: https://github.com/sibirrer/lenstronomy/blob/f3be287835bf42c4a344669f30a889e83278bc1f/lenstronomy/LensModel/Profiles/nfw.py#L305
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I am a bit confused, isn't there only one 's' value for each string? Unless you mean that I should code it so that it works when x and y are arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes, I meant when x is an array (you probably want to have the rotated coordinates in here)
You can see the integrated testing results here: https://app.travis-ci.com/github/sibirrer/lenstronomy/builds/234520911 |
Great that your current tests are passing. Here you see which lines of executable code has not yet been tested: https://coveralls.io/builds/41974068/source?filename=lenstronomy%2FLensModel%2FProfiles%2Fcosmic_string.py#L74 |
No description provided.