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

Fix a bug in how a image group name is determined #8426

Merged
merged 7 commits into from
Apr 19, 2024

Conversation

mcara
Copy link
Member

@mcara mcara commented Apr 17, 2024

It has been reported when an user used custom file names with the same common file name in several groups of images (i.e., with different group_id in the asn files). The algorithm for computing group names (_common_name()) may fail to produce a unique name for some groups resulting in these groups not being aligned.

This is not an issue for pipeline runs on original JWST file names but may affect grouping when user renames input models.

This PR is a further improvement on #8012

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

@mcara mcara added this to the Build 10.0.1 milestone Apr 17, 2024
@mcara mcara self-assigned this Apr 17, 2024
@mcara mcara requested a review from a team as a code owner April 17, 2024 08:03
@mcara mcara requested a review from mairanteodoro April 17, 2024 08:03
Copy link

codecov bot commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.30%. Comparing base (2fb073e) to head (71d91f1).
Report is 34 commits behind head on master.

❗ Current head 71d91f1 differs from pull request most recent head b4112c9. Consider uploading reports for the commit b4112c9 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #8426       +/-   ##
===========================================
- Coverage   75.31%   56.30%   -19.02%     
===========================================
  Files         474      390       -84     
  Lines       38965    38886       -79     
===========================================
- Hits        29345    21893     -7452     
- Misses       9620    16993     +7373     
Flag Coverage Δ
nightly ?

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@braingram
Copy link
Collaborator

Is there an advantage to using a name different from the group_id? I see 2 uses of _common_name and in both cases the same _common_name is used for all images in a group (these images will also share the same group_id).

@mcara
Copy link
Member Author

mcara commented Apr 17, 2024

Is there an advantage to using a name different from the group_id?

I don't know for sure. One consideration was to have a relatively short name so that it is easily readable in the log file (a bad alternative would be to concatenate all file names of models in a group.) The main issue is that group_id is not a property of the image models and not even of the ModelContainer: all ModelContainer does is to compute group_id and use this information to return nested lists of models. Given that this code receives a nested list, _common_name() is the answer to that problem.

Ideally, I think group_id should be a property of DataModel whether group_id is stored in meta (I am not sure it should) or whether it is computed on demand. Until then, I don't see an alternative to _common_name().

@braingram
Copy link
Collaborator

Thanks! ModelContainer currently dynamically sets meta.group_id for the models returned from models_grouped (see the below code):

if (hasattr(model.meta, 'group_id') and
model.meta.group_id not in [None, '']):
group_id = model.meta.group_id
else:
for param in unique_exposure_parameters:
params.append(getattr(model.meta.observation, param))
try:
group_id = (
'jw' + '_'.join(
[
''.join(params[:3]),
''.join(params[3:6]),
params[6],
]
)
)
model.meta.group_id = group_id
except TypeError:
model.meta.group_id = 'exposure{0:04d}'.format(i + 1)

@mcara mcara requested a review from braingram April 17, 2024 14:54
Copy link
Collaborator

@mairanteodoro mairanteodoro 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. Thanks, @mcara!

Copy link
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

LGTM! Have regtests been run with these changes?

@mcara
Copy link
Member Author

mcara commented Apr 17, 2024

LGTM! Have regtests been run with these changes?

running now

@mcara
Copy link
Member Author

mcara commented Apr 17, 2024

@mcara
Copy link
Member Author

mcara commented Apr 17, 2024

Thanks @braingram & @mairanteodoro

@braingram
Copy link
Collaborator

Happy to help!
Looks like the regtests all passed 🎉

@hbushouse hbushouse modified the milestones: Build 10.0.1, Build 11.0 Apr 19, 2024
@hbushouse hbushouse merged commit e4ddbcd into spacetelescope:master Apr 19, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants