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

Create obs_base and make obs_test work with it Tickets/dm 7578 #19

Merged
merged 1 commit into from Nov 1, 2016

Conversation

parejkoj
Copy link
Contributor

No description provided.

Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

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

Overall this looks fine

'3': {'visit': 3, 'filter': 'r'}
}
self.path_to_raw = os.path.join(self.data_dir, "raw", "raw_v1_fg.fits.gz")
self.result_ids = {'raw': 1, 'bias': 1, 'flat': 1}
Copy link
Contributor

@r-owen r-owen Sep 21, 2016

Choose a reason for hiding this comment

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

I find this name confusing; what kind of IDs does this contain? If it's a visit ID, then result_visits would probably be clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are exposureIds. I've renamed it. Thanks.

def setUp(self):
product_dir = getPackageDir('obs_test')
self.data_dir = os.path.join(product_dir, 'data', 'input')
self.Mapper = lsst.obs.test.TestMapper
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if you will find you want the class or an instance. If the class, please consider appending Class to the instance variable name, for clarity (self.MapperClass is clearly not an instance of a Mapper)

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've redone this to be a an instance, instantiated in the child class.

@parejkoj parejkoj force-pushed the tickets/DM-7578 branch 2 times, most recently from efcaefe to ab30e60 Compare September 24, 2016 00:03
super(TestObsTest, self).setUp()

self.butler = lsst.daf.persistence.Butler(root=self.data_dir)
self.mapper = lsst.obs.test.TestMapper(root=self.data_dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is breaking encapsulation. TestObsTest should only set things that it cares about; it should not have to set up things for its parent class. If something needs to be set up for the parent class, then you should call a method in the parent class to do that. I should be able to change the names of instance variables in the parent class without this subclass knowing anything about it (I know that's not always possible, but it's impossible to such a large extent here that I think it's a bad idea).

@parejkoj parejkoj force-pushed the tickets/DM-7578 branch 3 times, most recently from ed2edb9 to 3b16506 Compare October 12, 2016 23:38
Removed testMap.py, whose contents were lifted into obs_base mapper_tests.py
@parejkoj parejkoj merged commit 772b580 into master Nov 1, 2016
@ktlim ktlim deleted the tickets/DM-7578 branch August 25, 2018 06:16
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