-
Notifications
You must be signed in to change notification settings - Fork 30
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
Code updates for JWSTSIAF-284 #363
base: siaf-updates
Are you sure you want to change the base?
Code updates for JWSTSIAF-284 #363
Conversation
Update pysiaf with PRDOPSSOC-068
…arnings Fix deprecation warnings from importlib and numpy
"NRCA5_MASK210R", | ||
"NRCA2_TAMASK210R", | ||
"NRCA2_FSTAMASK210R", | ||
"NRCA5_{}STRIPE{}_DHS_F322W2".format(pix,stripe) |
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.
Fancy -- A little less readable in my opinion but if this is a request added by a team I won't say anymore 👍
@@ -17,9 +17,9 @@ def makeup_polynomial(order = 5): | |||
a = np.zeros(terms) | |||
|
|||
np.random.seed(seed=1) | |||
a[1] = 0.05 + 0.01 * np.random.rand(1) | |||
a[1] = 0.05 + 0.01 * np.random.rand() |
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.
@pbennet I see we are changing the dimensions of the coefficients here. Does this change anything downstream?
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 guess to be specific, I know that with numpy 2.0 there were updates to array operations, but is there a benefit to changing values of a
from float * single_value_array
to float * float
?
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 first case gives me the old warning
<ipython-input-8-7508f9c68cc9>:1: DeprecationWarning: Conversion of an array with ndim > 0 to a scalar is deprecated, and will error in future. Ensure you extract a single element from your array before performing this operation. (Deprecated NumPy 1.25.)
and the dimensions of both outputs look the same to me:
`In [10]: a[1] = 0.05 + 0.01 * np.random.rand(1)
:1: DeprecationWarning: Conversion of an array with ndim > 0 to a scalar is deprecated, and will error in future. Ensure you extract a single element from your array before performing this operation. (Deprecated NumPy 1.25.)
a[1] = 0.05 + 0.01 * np.random.rand(1)
In [11]: print(a.shape)
(3,)
In [12]: a[1] = 0.05 + 0.01 * np.random.rand()
In [13]: print(a.shape)
(3,)`
@@ -545,7 +545,7 @@ def prepend_rotation_to_polynomial(a, theta, verbose=False): | |||
for j in range(m-n-mu, m-mu+1): | |||
factor = (-1)**(m-n-mu) * choose(m-j, mu) * choose(j, m-n-mu) | |||
cosSin = c**(j+2*mu-m+n) * s**(2*m-2*mu-j-n) | |||
atrotate[m, n] = atrotate[m, n] + factor * cosSin * at[m, j] | |||
atrotate[m, n] = np.squeeze(atrotate[m, n] + factor * cosSin * at[m, j]) |
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 am looking at a two-dimensional array in python:
>>>: x = np.random.rand(3,3)
>>> print(x)
array([[0.8991704 , 0.47865258, 0.24120141],
[0.14969249, 0.59154556, 0.9030803 ],
[0.46856477, 0.45624738, 0.92141728]])
>>> x.squeeze()
array([[0.8991704 , 0.47865258, 0.24120141],
[0.14969249, 0.59154556, 0.9030803 ],
[0.46856477, 0.45624738, 0.92141728]])
It looks like the squeeze method doesn't change the shape/dimensions of the array, is this change necessary?
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 have some comments and questions about the changes proposed here before we merge but over all there isn't anything obvious that would halt an approval. @Witchblade101 I think that we should wait for @pbennet's response on this before moving forward.
Agreed |
@bmorris3 Can you answer @mfixstsci 's questions above? It looks like these issues stem from the changes you made in November that we forgot to merge into the siaf-updates branch. We're in the middle of preparing a SIAF update for NIRCam, so it's somewhat urgent. |
Added updated files from Paul Bennet for JWSTSIAF-284. Also includes outstanding changes from Main branch.