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

DM-37085: Add copyWith method to VisitInfo #669

Merged
merged 1 commit into from Nov 29, 2022
Merged

DM-37085: Add copyWith method to VisitInfo #669

merged 1 commit into from Nov 29, 2022

Conversation

fred3m
Copy link
Contributor

@fred3m fred3m commented Nov 23, 2022

No description provided.

tests/test_visitInfo.py Outdated Show resolved Hide resolved
hasSimulatedContent = self.hasSimulatedContent

return VisitInfo(
exposureId=0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't that be exposureId=exposureId? And doesn't that then need an accompanying block above like all the others? Or am I missing something about that being special somehow?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, or are you saying that because it's a copy, it no longer corresponds to the original expId, and therefore must be zeroed out? If so, might be worth setting a more obviously suspicious value like -1, and either way, a comment to say that's why it's hard-coded to zero.

Copy link
Contributor Author

@fred3m fred3m Nov 25, 2022

Choose a reason for hiding this comment

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

Oh, this was me being lazy and just trying to make the tests work in time for the end of the day. The issue is that exposureId is deprecated (see https://github.com/lsst/afw/blob/main/python/lsst/afw/image/_visitInfo.py#L31-L35), so VisitInfo has a getExposureId method, but not an exposureId property. Fixed this and updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha OK, thanks.

python/lsst/afw/image/_visitInfo.py Show resolved Hide resolved
@fred3m fred3m merged commit 2409686 into main Nov 29, 2022
@fred3m fred3m deleted the tickets/DM-37085 branch November 29, 2022 18:19
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.

None yet

2 participants