-
Notifications
You must be signed in to change notification settings - Fork 13
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
"Simple" example using romanisim-make-image for docs #57
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
=======================================
Coverage 90.00% 90.00%
=======================================
Files 15 15
Lines 1351 1351
=======================================
Hits 1216 1216
Misses 135 135 ☔ View full report in Codecov by Sentry. |
Looks good to me; feel free to merge you're ready. For this particular case one could imagine calling romanisim.image.simulate(...) directly, without the romanisim-make-image wrapper, but this is a nice approach as well, and doubles as a demo of the CLI. |
The motivation for using the CLI version in the above script instead of romanisim/scripts/romanisim-make-image Lines 62 to 92 in a6a8ecd
It may be worthwhile to prevent users from kicking the can down the road on learning these necessary inputs though, especially for this doc example. I'll edit with simulate !
|
@schlafly This updated version uses |
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.
Looks good to me. A few minor things you could drop that I think were part of the previous implementation.
# save the star catalog here: | ||
catalog_path = 'small-synthetic-catalog.ecsv' | ||
|
||
# write out the result to ASDF: | ||
output_path = 'small-synthetic-image.asdf' |
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.
I think this is vestigial?
faintmag=18, | ||
rng=UniformDeviate(seed=seed) | ||
) | ||
cat.write(catalog_path, overwrite=True) |
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.
I think this is vestigial?
asdf_file = asdf.AsdfFile() | ||
romanisimdict = {'simcatobj': simcatobj} | ||
asdf_file.tree = {'roman': im, 'romanisim': romanisimdict} |
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.
I think you could drop this, and show im['data'] below instead of asdf_file.tree['roman']['data'].
In learning to use
romanisim
, I found that the Running the simulation page in the docs explains the arguments, but does not have examples.Users want/will want to have a reproducible usage example in the docs, so I've added one here.