Skip to content

Merge from asdf feature branch #95

Open
0Navin0 wants to merge 12 commits intomainfrom
save_asdf
Open

Merge from asdf feature branch #95
0Navin0 wants to merge 12 commits intomainfrom
save_asdf

Conversation

@0Navin0
Copy link

@0Navin0 0Navin0 commented Mar 3, 2026

This PR avails an option to save the output file in ASDF format that satisfies roman_datamodels. This is a step towards migration from FITS to ASDF format. The changes include file format and metadata propagation to the output file. Further improvements can include assignment of specific values to the currently populated default-values (from create_fake_data) for some keys in the metadata, and an implementation of the output file name consistent with WFI File Naming Conventions.

@0Navin0
Copy link
Author

0Navin0 commented Mar 4, 2026

On commit 50b40ab50b40ab
Thanks for the cleanup work. Some imports are now missing in output.py because wcs_from_fits_header() method was moved from sca.py to output.py without its dependencies. I didn't copy that function to output.py because I couldn't make a decision as to which file should have this method, sca.py or output.py. I thought, as long as we are still documenting the use of this function in sca.py, everything related to this function is already imported in sca.py, so I just called it from RomanSCAImageBuilder instead of repeating so many lines in output.py. So I guess either you can reimport the dependencies again in output.py or just keep using RomanSCAImageBuilder for now.

Including these dependencies in output.py fixes the problem (Note that all these imports are already there in sca.py)

import astropy.coordinates
import astropy.units as u
import gwcs
from astropy import wcs as fits_wcs
from astropy.modeling.models import Mapping, Pix2Sky_TAN, Polynomial2D, RotateNative2Celestial, Shift
from gwcs import coordinate_frames as cf

@arunkannawadi
Copy link
Member

I'll move these imports too, thanks for the headsup. As to where this method should live, output.py makes the most sense now since that's where it is being used. If this is more useful, I can see this being moved to utils.py but that can happen later.

@arunkannawadi
Copy link
Member

FYI, this PR won't change sca.py. All relevant changes will be moved to output.py.

@arunkannawadi arunkannawadi force-pushed the save_asdf branch 4 times, most recently from 5796e4c to a17ab3b Compare March 5, 2026 04:38
Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

I ran the hack.yaml in the save_asdf branch of roman_imsim_testdata and confirmed that it produces an ASDF file that can be read with roman_datamodels. Kudos on this great work Boyan, Navin and Raul 🎉

@arunkannawadi
Copy link
Member

Requesting Axel and Sid to take a quick look as well - this could use more clean up, but I need to produce images in ASDF soon, so willing to cut corners as long as it works now.

Copy link
Contributor

@sidneymau sidneymau left a comment

Choose a reason for hiding this comment

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

I ran hack.yaml and things seem to look ok --

Python 3.14.3 | packaged by conda-forge | (main, Feb  9 2026, 21:56:02) [GCC 14.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import asdf
>>> import roman_datamodels as rdm
>>> dm = rdm.open("output/RomanWAS_new/images/truth/Roman_WAS_truth_J129_12909_4.asdf")
>>> dm.validate()

And the corresponding image looks reasonable
Image

A few changes/comments throughout. IMO, the use of create_fake_data is probably a blocker

tree.meta.pointing.target_aperture = (
f"{wcs_header['INSTRUME']}_CEN" # what kidn of aperture is wcs_header['RA_TARG']
)
tree.meta.pointing.target_ra = image.wcs.center.ra.deg # or wcs_header['RA_TARG']?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still uncertain? (also below)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe something to check, usually "target" refer to where the telescope is pointing which can be the center of the mosaic but necessarily. image.wcs.center can refer to the center of the SCA or mosaic. It most cases they are not the same. The "target" coordinates should be provided by the observation sequence file.
Probably not a big if not set correctly for simulations though..

Copy link
Author

Choose a reason for hiding this comment

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

meaning of meta.pointing.target_aperture:

title: Aperture Name Used for Pointing
description: Name of the aperture used to align the instrument to a position on the sky.

meaining of target_ra:

title: Right Ascension of the Target Aperture (deg)
description: Right ascension in units of degrees at the location of the target aperture.

I wasn't sure if one of these should go to pointing.target_ra: wcs_header["RA_TARG"] or image.wcs.header.header['CRVAL1'] or base['world_center'].ra.deg.

Also, please note that the origin differs between image and WCS pixel coordinates. And that creates some discrepancy between base['world_center'].ra.deg and image.wcs.header.header['CRVAL1'].

Please note the following -

# ==> But why is this value different? Is it expected?
ipdb> base['world_center'].ra.deg
10.832447108980707
# Versus
ipdb> image.wcs.header.header['CRVAL1']
np.float64(10.83242929473425)

----------------------------
ipdb> base['world_center'].dec.deg
-44.477474568888496
# Versus
ipdb> image.wcs.header.header['CRVAL2']
np.float64(-44.47749107477652)
----------------------------
# >>>>>>>>>>>> This is the issue
ipdb> for k in ['image_xsize', 'image_ysize', 'image_origin', 'image_center', 'image_bounds', 'wcs', 'world_center']:
    print(k, "==>", base[k])

image_xsize ==> 4088
image_ysize ==> 4088
image_origin ==> galsim.PositionI(1,1)
image_center ==> galsim.PositionD(2044.5,2044.5)
image_bounds ==> galsim.BoundsI(1,4088,1,4088)
# Versus
ipdb> base['wcs'].origin
galsim.PositionD(x=0.0, y=0.0)

# Versus # comment = right ascension of the target (deg) (J2000)
ipdb> wcs_header["RA_TARG"]
10.566 

Copy link
Collaborator

Choose a reason for hiding this comment

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

From what I see, I would recommend:

tree.meta.pointing.target_ra = wcs_header['RA_TARG']

Now the difference between base['world_center'] and image.wcs.header.header['CRVAL1'] is 0.5 pixels. This is because the pixel center in the WCS is defined as: n_pix/2 which is 2044 while the base['world_center'] is defined with respect to the image center which is 2044.5

tree.meta.ref_file.gain = tree.meta.ref_file.gain
# tree.meta.ref_file.inverselinearity = tree.meta.ref_file.inverselinearity
tree.meta.ref_file.linearity = tree.meta.ref_file.linearity
# tree.meta.ref_file.integralnonlinearity = tree.meta.ref_file.integralnonlinearity
Copy link
Member

Choose a reason for hiding this comment

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

I had to remove darkdecaysignal, inverselinearity and integralnonlinearity to make these work.

Copy link
Author

@0Navin0 0Navin0 Mar 6, 2026

Choose a reason for hiding this comment

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

I don't know what the problem was, since it was working fine for me. These are all default values, so I didn't care much when you commented it out.

Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

The unresolved comments are for @0Navin0 , @BoyanYin and @rbgdet to respond

Co-authored-by: Navin Chaurasiya <[email protected]>
Co-authored-by: Raul Teixera <[email protected]
.edu>
Co-authored-by: Boyan Yin <[email protected]>
Copy link
Collaborator

@aguinot aguinot left a comment

Choose a reason for hiding this comment

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

I did a comparison between fits and asdf and I found some differences.

  • First of all, the pixels are not the same and I don't understand why.. I used the same config file in the same environment I just saved to fits instead of asdf and I am not using gz compression. I found that 16% of the pixels are different with a mean difference of 1.4e-10 and a maximum difference of 7e-4.
  • The WCS are also different. I think (hope) that it comes from the +1 in the NAXIS. right now fits_wcs.pixel_to_world(100, 100) != asdf_wcs.pixel_to_world(100, 100) BUT fits_wcs.pixel_to_world(100, 100) == asdf_wcs.pixel_to_world(101, 101)
  • I really don't understand all the assignement of tree.meta.SOMETHING = tree.meta.SOMETHING. Why are most of the keys assigned to themselves?

Edit: sorry for requesting changes afterward, I didn't found a way to update my review.. The +1 in NAXIS is a problem that need to be resolved before merge otherwise the wcs are mostlikely wrong.

@0Navin0 0Navin0 closed this Mar 5, 2026
@0Navin0 0Navin0 reopened this Mar 5, 2026
Navin Chaurasiya added 2 commits March 6, 2026 11:00
noise.py => added an optional key "ignore_noise".

Not sure if leaving this one out was intentional? Please revert the
changes if use of "ignore_noise" is supposed to be deprecated.

sca.py => added a helpful comment telling to move a header card
assignement to WCS file in the future

output_asdf.py => cleanup
@0Navin0 0Navin0 requested a review from aguinot March 6, 2026 19:03
GWCS is by default 0-indexed. See
https://gwcs.readthedocs.io/en/latest/gwcs/user_introduction.html

The following inclusion of 0-indexing key in the yaml config file is
necesasry to have 0-indexing in detector.
image:
    index_convention: 0

Another minor change is to explicitly specify the type of image getting
saved. => tree.meta.exposure.type = "WFI_IMAGE"
This is to avoid deleting it along with all the default assignments by
mistake.
wcs_header.update(image.header)

tree = ImageModel.create_fake_data()

Choose a reason for hiding this comment

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

This function fills out default values for all of the fields that are adequate to make a valid file. You should only need to fill out new data in the tree that you have actually computed and where you care about the result. All of the lines that follow the pattern

tree.meta.product_type = tree.meta.product_type

are not doing anything and can be deleted---most of the mass of this function, lines 208 - 376 or so.

Re when using this is appropriate vs. not---the issue is that this is inventing a large amount of stuff---a large number of times, locations in orbits, investigator names, etc.. Conceptually the validation structure exists to make sure that this was all filled out correctly, and using create_fake_data(...) prevents that validation functionality from verifying that an attempt has been made to fill out all of the needed metadata. But for this code you likely do not want to be warned that you didn't enter a value for the status of the relative calibration system during this exposure, and indeed want some default garbage value and do not want to think more about this. That's what create_fake_data is for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this context, Eddie -- this makes sense, I guess our use case isn't quite addressed by the cautions against using create_fake_data from the documentation (and or I misinterpreted)

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.

Convert FITS to roman_datamodel, save asdf, assign simulation data and structure output filename

5 participants