EGaraldi / corecon

User-friendly collection of Reionization-related constraints
https://corecon.readthedocs.io/en/latest/
GNU General Public License v3.0
8 stars 5 forks source link

Suggestions and ideas for functionality #12

Closed Stefan-Heimersheim closed 4 months ago

Stefan-Heimersheim commented 4 years ago

Hello again,

while trying out the code I noted a few small things that don't work optimal. Those are not bugs, just some feedback - maybe you want to change things before the code is widely used (as it is much harder to break compatibility later).

1) The description field in the same dimension as the measurements is a non-obvious idea and a bit confusing when e.g. reading the tutorial or trying to write a loop yourself. Of course it can be solved by adding if k=="description": continue to every loop but it's not intuitive.

A possible solution would be a simple class instead of a dictionary for each constraint. Currently T0 = crc.get("T0") is a dictionary with keys 'description', 'Hiss et al. 2018' etc. I'm not a Python expert but I think a minimal class like

class constraint(dict):
    description = ""

could work. Then you have T0 still like a dictionary with T0['Hiss et al. 2018'] but looping through constraints is straightforward (T0.keys()) and the description can be consistently accessed with T0.description.

2) The format of data entries with err_up, err_up2, ... that are either filled with values, set to 0 or None, is a bit difficult. Especially when e.g. only 2 sigma errors are given or the confidence intervals do not correspond to 1 or 2 sigma. Also upper limits are not trivial -- how do I differentiate an 1 sigma from a 2 sigma upper limit?

I don't have a simple solution here. Maybe one could have a field for error(s) and a field for confidence levels, e.g. errors = [0.1, 0.15], confidence = [0.68, 0.95] for 1 and 2 sigma errors could work, but I'm not sure what is best here.

Best, Stefan

EGaraldi commented 2 years ago

Hi, I like the idea of inheriting from dict to separate the description field. This provides also more flexibility for the future. So, I've implemented it. Concerning your second point, I agree but also I do not have many great ways forward. In principle one could specify in the DataEntry description what the errors are about but it's not great. Maybe the best would be to provide only one error field (up and down), and then allow the user to put additional fields with the name they want.

EGaraldi commented 2 years ago

For now I have made err_up2 and err_down2 optional, since they were seldom used.