example_16: added color and negative blending prior

[mjmroz]

For both sources

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2e55c55) 83.61% compared to head (68a211d) 83.59%.
Report is 23 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #122      +/-   ##
==========================================
- Coverage   83.61%   83.59%   -0.03%     
==========================================
  Files          52       52              
  Lines        8271     8313      +42     
==========================================
+ Hits         6916     6949      +33     
- Misses       1355     1364       +9     

Flag	Coverage Δ
unittests	83.59% <ø> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rpoleski]

There are a few things to discuss or modify:

• Break _parse_fit_constraints() and _run_flux_checks_ln_prior() into shorter functions.
• Consider if proposed API is OK or should be changed - to be discussed with @rapoliveira .
• Add documentation - line 208 and below.
• Make it PEP8-compliant: preferably using flake8 software, e.g., remove duplicated empty lines.
• Change version to 0.35.0 or so.

[rpoleski]

@mjmroz If you want to finish this, then I suggest you start with 1st and 4th item on the list above. There is not much work needed.

[rpoleski]

@mjmroz Did you need negative blending flux prior for more than one dataset?

[rapoliveira]

My only sugggestion to this new API is to replace source_number and similar words to source_flux or data_number or data_label, so it's not confused with having more than one source star.

@mjmroz I have some minor suggestions to make the code more readable, we can chat in person if you want.

[rpoleski]

Hi.
I see progress, but still _parse_fit_constraints() is way too long. Also _run_flux_checks_ln_prior has to be made shorter. The _sumup_inside_prior() needs a docstring.

[rpoleski]

Sorry, closed by mistake. Reopening.

[rpoleski]

One more aspect that I think should be changed is how the user specifies datasets to be used. Instead of numbers (which are easy to confuse, e.g., when somebody adds a dataset to existing yaml file), I suggest to use labels that can be accessed via MulensData.plot_properties['label'], which defaults to the base of file name.

[rpoleski]

1. Where is the check that the two datasets don't have the same label?
2. inside = self...() in _run_flux_checks_ln_prior() are wrong, should be +=.
3. please use full words for variable names (e.g., index instead of idx).
4. Example improvement, around line 2260:

for i in arange(len(self._n_fluxes_per_dataset) - 1):
     inside += self._sumup_inside_prior(fluxes, key, inside, i)

@mjmroz

[mjmroz]

@rpoleski
I implemented a check for using different datasets to calculate the color ( #L1525 ). Did you mean that a general check to ensure MulensData.plot_properties['label'] is unambiguous is missing? I can add this as well.

Have you considered adding an option to declare different identifiers for datasets in the input YAML file that are not passed to MulensData.plot_properties['label']? One might need to have more than one dataset on the plot under one label but treat them differently in modeling.

Which way of declaring color prior in the YAML file do you prefer?

#negative_blending_flux_sigma_mag: [20., "OGLE I-band", "OB03235_MOA.txt"]
# Alternative sharp constraint:
# no_negative_blending_flux: True
#color constrains, where color=flux_S_dataset_k/flux_S_dataset_m: [gauss, mu ,sigma, k, m ,]
color : ["gauss", 25., 5. , "OGLE I-band" ,"OB03235_MOA.txt" ]

or

negative_blending_flux_sigma_mag: 20.  
#one can specify for which dataset soft constrain on blending flux should be applied: sigma dataset_number(s)
soft_negative_blending_flux:
    sigma_mag: 20.
    data_sets: ["OGLE I-band","OB03235_MOA.txt" ] #optional
# Alternative sharp constraint:
# no_negative_blending_flux: True
#color constrains, where color=flux_S_dataset_k/flux_S_dataset_m
#color:
#  settings : gauss mu sigma
#  color_from : [k, m]
color :
    settings: gauss 25. 5.  
    color_from : [ "OGLE I-band", "OB03235_MOA.txt"]

[rpoleski]

Line 1525 says settings[3] = self._get_no_of_dataset_by_lable(settings[3]) and I don't see how it would prevent two datasets to have the same label. There should be such a test if the user specifies the color constraint.

Have you considered adding an option to declare different identifiers ...

No. It's not needed at this point.

Preferred formats:

color: gauss 2.0 0.1 "OGLE V-band" "OGLE I-band"
# or:
color: [gauss, 2.0, 0.1, OGLE V-band, "OGLE I-band"]

i.e., IDs instead of numbers and distribution clearly specified. One aspect is missing at this point: do we want flux ratio or magnitude difference? I'll be for the latter because that is what astronomers are used to.

I also suggest:

5. lable -> label

[mjmroz]

I meant this line 1525 :) :
https://github.com/mjmroz/MulensModel/blob/c7c558b688b12abbf5be855e5a8a9372612edb69/examples/example_16/ulens_model_fit.py#L1525 : if settings[3] == settings[4]

As I wrote, it checks if one is trying to calculate color from the same datasets, for example, color = OGLE-I/OGLE-I. In our previous live conversation, when you asked if I have a check to ensure datasets' labels are different, I misunderstood you and thought you were asking about this one, so I said yes. I'm sorry for that.
In my code, there is no check if one label is used multiple times in the declaration of MulensData.plot_properties['label']. I will add it.

color: gauss 2.0 0.1 "OGLE V-band" "OGLE I-band"

or:

color: [gauss, 2.0, 0.1, OGLE V-band, "OGLE I-band"]

The first option is now implemented, but it requires importing modules like shlex to deal with quotations inside a str. I will use the latter one.

I'm using fluxes because it was the easiest way in my case. I needed a color prior when I was adding a second source to the model, so I already had results in fluxes for the first source. If you think it won't be the most common case, I will change to magnitudes or add an option for both:
color: [gauss, 2.0, 0.1, OGLE V-band, "OGLE I-band", "mag" or "flux"]

[rpoleski]

Current summary of what is missing:

• documentation around line 208,
• make _parse_fit_constraints() shorter,
• make _run_flux_checks_ln_prior() shorter,
• add function like _check_unique_datasets_labels() that raises an exception if labels are not unique,
• my comments from 27.05.2024,
• input in fluxes,
• at the end: flake8 compliance, version update.

Current format of input with shlex is ok. I've realized the other one (with a list) would require some more checks and there is no need for that now.
Astronomers very rarely use flux ratios for colors, so let's switch to magnitudes.

[rpoleski]

Thanks for all these updates and for correcting older spelling errors.

Two final corrections:

• please comment out line 42 in ob03235_2_full.yaml (because it has the same key as line 40)
• please change version number to 0.37.0 in line 42 of ulens_model_fit.py

I'll merge this PR afterwards. Thanks once more.