litebird / litebird_sim

Simulation tools for LiteBIRD
GNU General Public License v3.0
18 stars 13 forks source link

Some changes in the class Simulation and in IO #293

Closed paganol closed 5 months ago

paganol commented 5 months ago

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 commented 5 months ago

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

paganol commented 5 months ago

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:

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 commented 5 months ago

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 commented 5 months ago

It sounds good. I'll add it

paganol commented 5 months ago

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 commented 5 months ago

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!