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

"Simple" example using romanisim-make-image for docs #57

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bmorris3
Copy link
Contributor

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.

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (a6a8ecd) 90.00% compared to head (da99dc0) 90.00%.

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@schlafly
Copy link
Collaborator

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.

@bmorris3
Copy link
Contributor Author

The motivation for using the CLI version in the above script instead of simulate, was really that there are a bunch of tasks that the CLI version takes care of for you, which I wanted to avoid duplicating:

if args.radec is not None:
coord = SkyCoord(ra=args.radec[0] * u.deg, dec=args.radec[1] * u.deg,
frame='icrs')
wcs.fill_in_parameters(metadata, coord, boresight=args.boresight)
if args.date is not None:
metadata['exposure']['start_time'] = Time(
datetime.datetime(*args.date)).isot
date = Time(metadata['exposure']['start_time'], format='isot')
metadata['instrument']['detector'] = f'WFI{args.sca:02d}'
metadata['instrument']['optical_element'] = args.bandpass
metadata['exposure']['ma_table_number'] = args.ma_table_number
if args.catalog is None:
twcs = wcs.get_wcs(metadata, usecrds=args.usecrds)
rd_sca = twcs.toWorld(galsim.PositionD(
galsim.roman.n_pix / 2, galsim.roman.n_pix / 2))
cat = catalog.make_dummy_table_catalog(
rd_sca, bandpasses=[args.bandpass], nobj=args.nobj)
else:
log.warning('Catalog input will probably not work unless the catalog '
'covers a lot of area or you have thought carefully about '
'the relation between the boresight and the SCA locations.')
cat = Table.read(args.catalog)
if args.previous is not None:
persist = persistence.Persistence.read(args.previous)
else:
persist = persistence.Persistence()

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!

@bmorris3
Copy link
Contributor Author

@schlafly This updated version uses simulate rather than the unnecessary check_call.

@bmorris3 bmorris3 marked this pull request as ready for review June 15, 2023 17:49
@bmorris3 bmorris3 requested a review from a team as a code owner June 15, 2023 17:49
Copy link
Collaborator

@schlafly schlafly left a 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.

Comment on lines +119 to +123
# save the star catalog here:
catalog_path = 'small-synthetic-catalog.ecsv'

# write out the result to ASDF:
output_path = 'small-synthetic-image.asdf'
Copy link
Collaborator

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)
Copy link
Collaborator

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?

Comment on lines +152 to +154
asdf_file = asdf.AsdfFile()
romanisimdict = {'simcatobj': simcatobj}
asdf_file.tree = {'roman': im, 'romanisim': romanisimdict}
Copy link
Collaborator

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'].

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