Remove is_ul column in FluxPointsEstimator if no upper limit is defined

[Astro-Kirsty]

Description
The default for selection_optional in FluxPointsEstimator is None. This means that the is_ul column shows all False. This current behaviour seems non-intuitive because it is not False, rather it has not been calculated.

This PR proposes an alternative, such that if has_ul is False, then the is_ul column will be removed, as to avoid confusion for the user.

[adonath]

I think this would be even simpler:

Suggested change

	table["is_ul"] = self.is_ul.data[idx]
	if not self.has_ul:
	table.remove_columns("is_ul")
	if self.has_ul:
	table["is_ul"] = self.is_ul.data[idx]

[Astro-Kirsty]

This was also my initial idea, however, this section of code already creates the column is_ul and fills that column with data.

gammapy/gammapy/estimators/points/core.py

Lines 361 to 364 in 0655a46

	for quantity in self.all_quantities(sed_type=sed_type):
	data = getattr(self, quantity, None)
	if data:
	table[quantity] = data.quantity[idx]

Open to further suggestion

[adonath]

You are right, just leave the suggestion open....

[registerrier]

Thanks @Astro-Kirsty . This looks good. A test for this specific situation should be added .

[registerrier]

Thanks @Astro-Kirsty . See inline comments.

[codecov]

Codecov Report

Merging #4546 (4286d6c) into main (e45cb44) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #4546   +/-   ##
=======================================
  Coverage   95.05%   95.06%           
=======================================
  Files         221      221           
  Lines       31448    31459   +11     
=======================================
+ Hits        29893    29905   +12     
+ Misses       1555     1554    -1     

Impacted Files	Coverage Δ
gammapy/estimators/points/core.py	92.18% <100.00%> (+0.06%)	⬆️
gammapy/estimators/points/tests/test_core.py	95.17% <100.00%> (+0.19%)	⬆️

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[registerrier]

Thanks @Astro-Kirsty .

the test was not testing anything. I have left an inline suggestion to make it work: you have to create a new table from the FluxPoints object and check that is_ul is not one of its columns.

[registerrier]

Thanks @Astro-Kirsty . No further comment on my side.