-
Notifications
You must be signed in to change notification settings - Fork 171
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
Updated references to datamodels in stpipe, comments. #9
Updated references to datamodels in stpipe, comments. #9
Conversation
@@ -602,7 +602,7 @@ def get_reference_file_model(self, input_file, reference_file_type): | |||
|
|||
Returns | |||
------- | |||
reference_file_model : jwst_lib.models.ModelBase instance | |||
reference_file_model : jwst.datamodels.ModelBase instance | |||
A model to access the contents of the reference file. | |||
""" | |||
from jwst import datamodels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be "from .. import datamodels"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure that import was already there and is not really part of this pull request. I have no strong beliefs about which is best, relative or absolute. Would you like another pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can change the import in this existing branch and push again to origin. It will show up as a separate commit. But yeah, we want to use relative imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consensus during today's hackathon seemed to be that relative imports were preferred, just in case someone accidentally has something else defined as "jwst".
OK, not sure if you guys saw my additional commit, but I went in and changed all the existing datamodels imports from absolute to relative. I should add, I pushed the commit to jaytmiller and it auto-magically showed up here, no need to make a new branch. |
👍 |
Get my repo up to date
…l_replace fixes to save individual models with expected names
I updated two modules in stpipe from references to jwst_lib.models to jwst.datamodels. The changes involved comments and a log message.
Based on feedback from Howard, I also changed all the existing datamodels imports from absolute to relative.