-
Notifications
You must be signed in to change notification settings - Fork 13
Added missing cmds to currsys calls in scopesim codebase #372
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -471,8 +471,8 @@ class SpectralTraceListWheel(Effect): | |
| } | ||
| _current_str = "current_trace_list" | ||
|
|
||
| def __init__(self, cmds=None, **kwargs): | ||
| super().__init__(cmds, **kwargs) | ||
| def __init__(self, **kwargs): | ||
| super().__init__(**kwargs) | ||
| check_keys(kwargs, self.required_keys, action="error") | ||
|
|
||
| params = { | ||
|
|
@@ -493,7 +493,6 @@ def __init__(self, cmds=None, **kwargs): | |
| fname = str(path).format(name) | ||
| self.trace_lists[name] = SpectralTraceList(filename=fname, | ||
| name=name, | ||
| cmds=self.cmds, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idem, it is better to be explicit, why did you remove this?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was removed because cmds was also found in the kwargs. So to avoid doubling up on passing arguments, I left everything in kwargs |
||
| **kwargs) | ||
|
|
||
| def apply_to(self, obj, **kwargs): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -367,7 +367,7 @@ def make_image_hdu(self, use_photlam=False): | |
| [ph s-1 pixel-1] or PHOTLAM (if use_photlam=True) | ||
|
|
||
| """ | ||
| spline_order = from_currsys("!SIM.computing.spline_order") | ||
| spline_order = from_currsys("!SIM.computing.spline_order", self.cmds) | ||
|
|
||
| # Make waveset and canvas image | ||
| fov_waveset = self.waveset | ||
|
|
@@ -393,7 +393,7 @@ def make_image_hdu(self, use_photlam=False): | |
| spline_order=spline_order) | ||
|
|
||
| for flux, weight, x, y in self._make_image_tablefields(fluxes): | ||
| if from_currsys(self.meta["sub_pixel"]): | ||
| if from_currsys(self.meta["sub_pixel"], self.cmds): | ||
| # These x and y should not be arrays when sub_pixel is | ||
| # enabled, it is therefore not necessary to deploy the fix | ||
| # below in the else-branch. | ||
|
|
@@ -493,7 +493,7 @@ def _make_cube_tablefields(self, specs): | |
| xpix, ypix = imp_utils.val2pix(self.header, xsky, ysky) | ||
| flux_vector = specs[row["ref"]].value * row["weight"] / self.pixel_area | ||
|
|
||
| if from_currsys(self.meta["sub_pixel"]): | ||
| if from_currsys(self.meta["sub_pixel"], self.cmds): | ||
| xs, ys, fracs = imp_utils.sub_pixel_fractions(xpix, ypix) | ||
| for i, j, k in zip(xs, ys, fracs): | ||
| yield flux_vector * k, i, j | ||
|
|
@@ -559,14 +559,14 @@ def make_cube_hdu(self): | |
| [ph s-1 AA-1 arcsec-2] # as needed by SpectralTrace | ||
|
|
||
| """ | ||
| spline_order = from_currsys("!SIM.computing.spline_order") | ||
| spline_order = from_currsys("!SIM.computing.spline_order", self.cmds) | ||
|
|
||
| # 1. Make waveset and canvas cube (area, bin_width are applied at end) | ||
| # TODO: Why is this not self.waveset? What's different? | ||
| wave_unit = u.Unit(from_currsys("!SIM.spectral.wave_unit")) | ||
| wave_unit = u.Unit(from_currsys("!SIM.spectral.wave_unit"), self.cmds) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also broken. |
||
| fov_waveset = np.arange( | ||
| self.meta["wave_min"].value, self.meta["wave_max"].value, | ||
| from_currsys("!SIM.spectral.spectral_bin_width")) * wave_unit | ||
| from_currsys("!SIM.spectral.spectral_bin_width"), self.cmds) * wave_unit | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is broken.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. Curious. |
||
| fov_waveset = fov_waveset.to(u.um) | ||
|
|
||
| # TODO: what's with this code?? | ||
|
|
@@ -613,7 +613,7 @@ def make_cube_hdu(self): | |
| # PHOTLAM = ph/s/cm-2/AA | ||
| # area = m2, fov_waveset = um | ||
| # SpectralTrace wants ph/s/um/arcsec2 --> get rid of m2, leave um | ||
| area = from_currsys(self.meta["area"]) # u.m2 | ||
| area = from_currsys(self.meta["area"], self.cmds) # u.m2 | ||
| canvas_cube_hdu.data *= area.to(u.cm ** 2).value | ||
| canvas_cube_hdu.data *= 1e4 # ph/s/AA/arcsec2 --> ph/s/um/arcsec2 | ||
|
|
||
|
|
||
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.
Why did you undo these changes? It is better to make the
cmdsargument explicit and limit the use ofkwargsto arguments you don't expect.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.
Yep. I get that. But this ends in a bigger oroject of making sure every effect object has the explicit argument cmds in the init call