-
Notifications
You must be signed in to change notification settings - Fork 74
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
Fix NaN handling in cube fitting #3191
Changes from all commits
fefd283
db5ed01
7cec815
6511530
4252e8a
163e3dd
95f9fdb
3832873
766df6a
ebd3572
862dd87
f1e7898
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 |
---|---|---|
|
@@ -493,7 +493,7 @@ def _initialize_model_component(self, model_comp, comp_label, poly_order=None): | |
masked_spectrum.flux[~mask] if mask is not None else masked_spectrum.flux) | ||
|
||
# need to loop over parameters again as the initializer may have overridden | ||
# the original default value | ||
# the original default value. | ||
for param_name in get_model_parameters(model_cls, new_model["model_kwargs"]): | ||
param_quant = getattr(initialized_model, param_name) | ||
new_model["parameters"].append({"name": param_name, | ||
|
@@ -917,6 +917,12 @@ def _fit_model_to_cube(self, add_data): | |
|
||
# Apply masks from selected spectral subset | ||
spec = self._apply_subset_masks(spec, self.spectral_subset) | ||
# Also mask out NaNs for fitting. Simply adding filter_non_finite to the cube fit | ||
# didn't work out of the box, so doing this for now. | ||
if spec.mask is None: | ||
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. Is this new logic tested? 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. The 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. I added a test that should cover the existing spec.mask case as well (that mask would come from a spectral subset). |
||
spec.mask = np.isnan(spec.flux) | ||
else: | ||
spec.mask = spec.mask | np.isnan(spec.flux) | ||
|
||
try: | ||
fitted_model, fitted_spectrum = fit_model_to_spectrum( | ||
|
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.
But
np.nanmean
would still return NaN ifx
is all NaNs. Is this something we need to handle?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.
Honestly this probably didn't need to change to
nanmean
since there shouldn't be NaN x (spectral axis) values. The y_mean case above is the important one.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.
Same question for
y
also.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 know why someone would try to fit a cube of all NaNs but I think it's reasonable for it to not work if they do 🤷
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.
But didn't you say multiprocessing would swallow the exception, causing confusion?
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 could imagine someone did a bad subset that encompass all NaNs in the cube and then try to fit it. It is possible?