-
Notifications
You must be signed in to change notification settings - Fork 8
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
V3.0.3: getObsAtmo #142
V3.0.3: getObsAtmo #142
Conversation
jeremyneveu
commented
Nov 7, 2023
•
edited
Loading
edited
- use getObsAtmo if available
Pull Request Test Coverage Report for Build 7248394356
💛 - Coveralls |
spectractor/fit/fit_multispectra.py
Outdated
@@ -77,7 +77,7 @@ def _build_test_sample(nspectra=3, aerosols=0.05, ozone=300, pwv=5, angstrom_exp | |||
>>> _build_test_sample(3) | |||
""" | |||
spec = Spectrum("./tests/data/reduc_20170530_134_spectrum.fits") | |||
targets = ["HD111980", "MU COL", "ETADOR"] | |||
targets = ["HD111980", "HD111980"] #, "MU COL", "ETADOR", "HD205905", "HD142331", "HD160617"] |
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 don't understand why you have the same target twice, or why the others are left commented out (I'd either remove them, or put them in, depending on whether they're needed or not)
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.
OK, I simplify the code.
# if A1s.size > 1: | ||
# m = 1 | ||
# A1s[0] = m * A1s.size - np.sum(A1s[1:]) | ||
# self.params.values[self.A1_first_index] = A1s[0] |
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.
As always, I'd encourage delete of keep, but not leaving commented code, but it's up to you, of course.
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.
Yes, but this one I will keep it because not sure of my code. fit_multispectra.py
is still under construction and test
spectractor/fit/fit_multispectra.py
Outdated
tmp_x, tmp_model, tmp_model_err = self.simulate(*tmp_p) | ||
for s in range(model.shape[0]): | ||
J[ip].append((tmp_model[s] - model[s]) / epsilon[ip]) | ||
# if "XXXX_" not in self.params.labels[ip]: |
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.
Again, if this check is no longer necessary, why leave this in?
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.
OK, removed
spectractor/fit/fit_multispectra.py
Outdated
#epsilon = np.array([0.0001, 1e-3, 5, 0.01, 0.01]) | ||
#epsilon = np.array(list(epsilon) + [1e-3] * fit_workspace.nspectra + [1e-4] * fit_workspace.nspectra) |
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.
And again.
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.
Removed
@@ -614,7 +642,7 @@ def simulate(self, ozone, pwv, aerosols, angstrom_exponent=None): | |||
... title=a.title, label=a.label) | |||
>>> if parameters.DISPLAY: plt.show() | |||
""" | |||
if angstrom_exponent is not None and angstrom_exponent > 0: | |||
if angstrom_exponent is not None and angstrom_exponent < 0: |
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 not sure I understand the change from >
to <
. The error message makes it sound like anything that is not None
should cause a problem, but this check is doing something different than the words suggest.