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-24466: Can't run processCcd on multiple CCDs #270

Merged
merged 5 commits into from Apr 17, 2020
Merged

Conversation

plazas
Copy link
Contributor

@plazas plazas commented Apr 15, 2020

No description provided.

"""Return the linearizer.
"""Return a Linearizer() for a particular detector. Linearizer() applies to
one detector only (i.e., it is detector-specific). When processing multiple
detectors, a new instance of Lineaizer() will be created each time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please make this docstring follow the numpydoc guidelines?

It would also be helpful to include a little more information about the Linearizer that is being returned.

Specifically, I'd suggest something like:

"""Return a linearizer for the given detector.

On each call, a fresh instance of `Linearizer` is returned; the caller is responsible for initializing it
appropriately for the detector.

Parameters
----------
`datasetType`: ...
`pythonType`: ...
`butlerLocation`: ...
`dataId`: ...

Notes
-----
Linearizers are not saved to persistent storage; rather, they are managed entirely in memory.
On each call, this function will return a new instance of `Linearizer`, which must be managed
(including setting it up for use with a particular detector) by the caller. Calling
`bypass_linearizer` twice for the same detector will return _different_ instances of `Linearizer`,
which share no state.
"""

lin1 = self.butler.get('linearizer', ccdnum=1)
lin2 = self.butler.get('linearizer', ccdnum=2)
self.assertTrue(lin1)
self.assertTrue(lin2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure these assertTrues mean much — what does it mean for a Linearizer to be True?

If you just want to check that you have an object of some sort, I'd suggest assertIsNotNone.

Copy link
Member

@timj timj Apr 17, 2020

Choose a reason for hiding this comment

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

Aren't you wanting to test that the instances are distinct? (so checking that id(lin1) != id(lin2) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

that's the next line...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change it to assertIsNotNone. The original code in this test (before my modifications) had assertTrue.

@plazas plazas merged commit 6a9ca1a into master Apr 17, 2020
@timj timj deleted the tickets/DM-24466 branch February 25, 2021 17:43
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

3 participants