logic for checking if all fluxes are fixed

[rpoleski]

@jenniferyee recently made this commit: 6836cbd646fbb02a252a15368f5db9b04b86cbbe. I've found it complicated and thought how to simplify this code.

First, it seems impossible to have self.fix_source_flux of float type, because __init__ says: self.fix_source_flux = [fix_source_flux] .

Second, I've looked at self.fix_source_flux - according to what we decided long time ago it should be defined with @property so that it has proper documentation. Also think a bit about it, because if the user provides float and then access it, they will get a list with a float.

Third, I've found it somehow strange that it can be set to False but not to True. I would make it None instead of False. But this would require version 3.0.

Fourth, I've tried to simplify the code and came up with:

        if isinstance(self.fix_blend_flux, float):
            if self.fix_source_flux is not False and False not in self.fix_source_flux:
                self._calculate_magnifications(bad=False)
                self._blend_flux = self.fix_blend_flux
                self._source_fluxes = self.fix_source_flux
                return

It passes current tests. Do you agree with such change? Maybe we should add a private property self._are_all_fluxes_fixed of bool type and change it to if self._are_all_fluxes_fixed:...?

[jenniferyee]

Line references are for commit 1666e2b

First, it seems impossible to have self.fix_source_flux of float type, because __init__ says: self.fix_source_flux = [fix_source_flux] .

This seems to be just a matter of removing "float" from fitdata.py Line 289.

@rpoleski agree?

Second, I've looked at self.fix_source_flux - according to what we decided long time ago it should be defined with @property so that it has proper documentation. Also think a bit about it, because if the user provides float and then access it, they will get a list with a float.

This parallels the problem we have with self.source_flux, which is stored as a list, but returns a single value of n_sources = 1. If we make fix_source_flux a @property, as you propose, we could handle this as in

fitdata.py, Line 781:

        if self._model.n_sources == 1:
            return self.source_fluxes[0]

@rpoleski What do you think of this solution? Also, should we make fix_blend_flux an @property?

Third, I've found it somehow strange that it can be set to False but not to True. I would make it None instead of False. But this would require version 3.0.

I agree in retrospect that None would make sense in this case.

@rpoleski Can we change it to default to None but allow it to continue accepting False as an argument without changing version numbers?

Fourth, I've tried to simplify the code and came up with:

        if isinstance(self.fix_blend_flux, float):
            if self.fix_source_flux is not False and False not in self.fix_source_flux:
                self._calculate_magnifications(bad=False)
                self._blend_flux = self.fix_blend_flux
                self._source_fluxes = self.fix_source_flux
                return

It passes current tests. Do you agree with such change? Maybe we should add a private property self._are_all_fluxes_fixed of bool type and change it to if self._are_all_fluxes_fixed:...?

So, this is to replace fitdata.py Lines 289--304?

I think it needs parentheses:

>             if self.fix_source_flux is not False and False not in self.fix_source_flux:

-->

           if (self.fix_source_flux is not False) and (False not in self.fix_source_flux):

Also, I can't find unit tests for binary sources with one source fixed and the other free. So, we need to make sure the unit tests cover all possibilities:

• one source:

  • free blending, free source flux
  • fixed blending, free source flux
  • free blending, fixed source flux
  • both fixed

• two sources:

  • free blending, both source fluxes free
  • fixed blending, both source fluxes free
  • free blending, one source flux fixed, the other free (possibly swapped as well)
  • fixed blending, one source flux fixed, the other free (possibly swapped as well)
  • free blending, both source fluxes fixed
  • all fluxes fixed
  • Probably also needs tests for source flux ratios.

**So action items:

1. Expand unit tests
2. Make fix_source_flux a property**

Then, implement @rpoleski 's simplification of Lines 289--304.

[jenniferyee]

I started updating the unit tests in commit cb68187. All I have done so far is make BinarySourceTest more flexible so we can test arbitrary values of the fix parameters. I have not started implementing any new tests. Also, it made me realize that we want to test both the cases where the fix values equal the input and where they equal arbitrary values. This will require a different set of tests.

[jenniferyee]

Unit Tests have been expanded/refactored to cover all cases. See commits feeab36c965ae236ff30439d76bf1d4b2b6213fc and e9964fe

[rpoleski]

This seems to be just a matter of removing "float" from fitdata.py Line 289. @rpoleski agree?

No.

Also, should we make fix_blend_flux an @Property?

Yes. Good point!

If we make fix_source_flux a @Property, as you propose, we could handle this as in fitdata.py, Line 781: ...

@rpoleski Can we change it to default to None but allow it to continue accepting False as an argument without changing version numbers?

No to both. There may be a user that made their code dependent on False or a list value of .self.fix_source_flux.

Then, implement @rpoleski 's simplification of Lines 289--304.

I've checked that we can do that now and all tests pass.

Adding parenthesis - it doesn't matter in that case, so add them if you think it looks better.