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.

Sign up for GitHub

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

Jump to bottom

Incompatibility between roman_datamodels and stpipe attribute assignment #343

Open
braingram opened this issue Apr 19, 2024 · 6 comments · May be fixed by spacetelescope/stpipe#195
Open

Incompatibility between roman_datamodels and stpipe attribute assignment #343

braingram opened this issue Apr 19, 2024 · 6 comments · May be fixed by spacetelescope/stpipe#195
Assignees
@braingram

Comments

@braingram
Copy link
Collaborator

braingram commented Apr 19, 2024 •
edited
Loading

If a step is skipped (by the skip attribute being set to True before the step is called) stpipe will assign 'SKIPPED' to the model metadata:
https://github.com/spacetelescope/stpipe/blob/d20b6429cafe0a4ca846fd43af7b6a61241e632c/src/stpipe/step.py#L484-L486
Including a call like the follow (for tweakreg for example):

model['meta.cal_step.tweakreg'] = 'SKIPPED'

However this assignment does not set model.meta.cal_step.tweakreg (the expected metadata field) for roman datamodels.

import roman_datamodels.maker_utils as maker_utils, roman_datamodels.datamodels as dm
im = dm.ImageModel(maker_utils.mk_level2_image())
assert im.meta.cal_step.tweakreg == 'INCOMPLETE'   # the expected value is 'INCOMPLETE' on creation

im['meta.cal_step.tweakreg'] = 'SKIPPED'  # assign as would be done in stpipe for a skipped step

assert im.meta.cal_step.tweakreg == 'INCOMPLETE'  # the expected value was not updated
assert im._instance['meta.cal_step.tweakreg'] == 'SKIPPED'  # the above assignment created a key with the "dotted" string
@braingram braingram mentioned this issue Jun 27, 2024
Open
@emolter emolter mentioned this issue Jun 27, 2024
Merged
8 tasks
@braingram
Copy link
Collaborator Author

@schlafly Would you consider this a roman_datamodels bug?

Is it planned to allow assignments using the "dotted" string, or perhaps should this cause an error (instead of silently creating a new key value pair in the _instance) and stpipe updated to not used an assignment with a "dotted" string?

@schlafly
Copy link
Collaborator

schlafly commented Jun 27, 2024 via email

@braingram braingram mentioned this issue Aug 5, 2024
Closed
6 tasks
@braingram braingram mentioned this issue Aug 23, 2024
Merged
6 tasks
@braingram braingram linked a pull request Oct 9, 2024 that will close this issue
Open
7 tasks
@braingram
Copy link
Collaborator Author

There are additional factors that make the automatic skip setting in stpipe not work. Even if the attribute access is fixed most romancal steps do not define a class_alias. For example, refpix doesn't define one. When elp is run with refpix.skip=True the cal_step.refpix attribute of the model is not set because stpipe doesn't know to use refpix (it expects the class_alias to match the cal_step.<identifier>: https://github.com/spacetelescope/stpipe/blob/ecd5d162be425c24db2498b34bcbaeccec4ac557/src/stpipe/step.py#L511

@schlafly was it a design decision to not use class_alias for romancal steps? There are a few that have them (resample, tweakreg, etc) but many do not. I must admit that I don't have a full understanding of what class_alias does but I think it also allows strun resample to call romancal.steps.ResampleStep (which could be problematic if a user has both jwst and romancal installed).

I think the options for resolving this issue are either:

  • define class_alias for all romancal steps and have them match the cal_step.<identifier>
  • modify stpipe to not require a class_alias (and have some other API) for setting cal_step status

@schlafly
Copy link
Collaborator

schlafly commented Oct 9, 2024

@ddavis-stsci will know the design decisions better than I. I doubt that this was a conscious decision, and so we could go ahead and add them. But I don't have a solution for Webb & Roman sharing class_aliases.

@ddavis-stsci
Copy link
Collaborator

I think that we should define class_alias for all the steps and take advantage of the stpipe infrastructure.

@braingram braingram mentioned this issue Oct 24, 2024
Merged
7 tasks
@braingram
Copy link
Collaborator Author

See spacetelescope/stpipe#202 for a possible solution to resolving class_alias conflicts between jwst and romancal.

@braingram braingram mentioned this issue Nov 12, 2024
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@braingram braingram

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

standardize setattr use in skip braingram/stpipe
3 participants
@braingram @schlafly @ddavis-stsci