Add FindRadialSurface.py #6300
Add FindRadialSurface.py #6300
Conversation
|
@isaaclegred FYI |
|
@nilsdeppe can you take this one? It's for BNS ID. |
nilsdeppe
left a comment
There was a problem hiding this comment.
LGTM, a few suggestions that you can squash immediately
| "-l", | ||
| type=int, | ||
| required=True, | ||
| help="Max l-mode for the horizon search.", |
There was a problem hiding this comment.
Several occurrences of horizon -> surface
| if np.all(filled): | ||
| break | ||
| if not np.all(filled): | ||
| raise ValueError("Unable to find the surface.") |
There was a problem hiding this comment.
Would there be any value in printing out any additional info? Or maybe a verbosity flag to allow printing more? E.g. did no rays succeed or only some not succeed?
There was a problem hiding this comment.
Changed to this:
ValueError: Unable to find the surface where Field = 0.2 with center [0.0, 0.0, 0.0]. Missing values for 325 / 325
radial rays. One of the missing rays goes through: [0.01771545 0. 0.09841831]
| if output_surfaces_file: | ||
| assert output_coeffs_subfile or output_coords_subfile, ( | ||
| "Specify either 'output_coeffs_subfile' or 'output_coords_subfile'" | ||
| " or both." | ||
| ) |
There was a problem hiding this comment.
Can you move this check to the beginning of the function so that you can learn this info before we do a bunch of work trying to find the surface? I would be a bit annoyed if I had to wait to find a surface (I don't know how long it'll take) and then got this...
Used to find the surface of a star in volume data, then deform the domain to conform to the surface.
e38de6e to
127bad4
Compare
|
Done! Thanks for your review! |
Proposed changes
Used to find the surface of a star in volume data, then deform the domain to conform to the surface. This is needed for BNS initial data.
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments