Some changes in the class Simulation and in IO

[paganol]

This PR moves read_observations and write_observations from io.py to simulations.py, it should make the usage of the high level interface of the class Simulation more straightforward. It contains also small changes in the interface of make_binned_map in the same class.

[paganol]

This might break e2e-simulation but we'll need to update it anyway before the next round of simulations.

[ziotom78]

Wouldn't it be better to leave write_observations and read_observations where they are and add similar methods to the Simulation class that are tiny wrappers to them? In this way we avoid introducing a breaking change, and it's easier to use the low-level “naked” functions in small test code without the need to instantiate a full Simulation object.

This would be similar to what we did for several other functions:

• Simulation.add_dipole is a wrapper to add_dipole_to_observations;
• Simulation.add_noise is a wrapper to add_noise_to_observations;
• Simulation.apply_gaindrift is a wrapper to apply_gaindrift_to_observations;
• Simulation.make_binned_map is a wrapper to make_binned_map (same name!);
• Simulation.make_destriped_map is a wrapper to make_destriped_map (same name!).

[paganol]

I had the same thought at the beginning but then I realized that lbs.write_observations is already a wrapper to lbs.write_list_of_observations, so adding sim.write_observations would add a third unnecessary layer. With this PR we avoid this and the structure is:

• Simulation.write_observation is a wrapper to write_list_of_observations;
• Simulation.read_observation is a wrapper to read_list_of_observations;

which is coherent with

This would be similar to what we did for several other functions:

Simulation.add_dipole is a wrapper to add_dipole_to_observations;
Simulation.add_noise is a wrapper to add_noise_to_observations;
Simulation.apply_gaindrift is a wrapper to apply_gaindrift_to_observations;
Simulation.make_binned_map is a wrapper to make_binned_map (same name!);
Simulation.make_destriped_map is a wrapper to make_destriped_map (same name!).

[ziotom78]

I had the same thought at the beginning but then I realized that lbs.write_observations is already a wrapper to lbs.write_list_of_observations, so adding sim.write_observations would add a third unnecessary layer.

Oh, I see, I missed that fact.

Still, I think it would be better if we leave a small stub in io.py with the @deprecated macro, so that people still using that call will get a nice warning message and a tip about how to update their code, but the call will still work:

from deprecation import deprecated

@deprecated(
    deprecated_in="0.11",
    current_version=litebird_sim_version,
    details="Use Simulation.write_observations",
)
def write_observations(
    sim: Simulation,
    subdir_name: Union[None, str] = "tod",
    include_in_report: bool = True,
    *args,
    **kwargs,
) -> List[Path]:
    # Here we call the method we've just moved inside Simulation
    sim.write_observations(…)

(The real implementation of write_observations and read_observations stays where you've just moved it, of course.)

When we'll release version 0.12 we'll just chop off these two stubs, but simulations.py will stay the same.

[paganol]

It sounds good. I'll add it

[paganol]

Hi @ziotom78, I had some problems with circular import. I had to remove from .simulations import Simulation in io.py. A part from that I implemented what you suggested.

[ziotom78]

Hi @ziotom78, I had some problems with circular import. I had to remove from .simulations import Simulation in io.py. A part from that I implemented what you suggested.

You know, this reminds me of a similar problem I had long time ago. It might be that the weird choice of not having write_observations be a method of Simulation was due to this… Great you solved it easily!