search

hpc4cmb / toast

Time Ordered Astrophysics Scalable Tools
Other
44 stars 39 forks source link

Missing file argument in Focalplane constructor #546

Open giuspugl opened 2 years ago

giuspugl commented 2 years ago

the latest toast3 version is missing a file argument in case we want to provide a focalplane file , althouhg it is expected to be there from the docstring . see https://github.com/hpc4cmb/toast/blob/60bcff20775dee83696d44e151f8a8d3ad3b0038/src/toast/instrument.py#L441

is it just a bug ?

keskitalo commented 2 years ago

Not a bug, just a different design. Now you instantiate the focalplane and use the load_hdf5() method. For an example, see toast_sim_ground.

tskisner commented 2 years ago

Although the design is intentional, @giuspugl is correct that the docstring was stale. I just fixed that in the branch where I am working on the documentation.

The reasoning behind the design choice was that it was too easy to accidentally open the same HDF5 file from multiple processes causing file contention. I was trying to make the user decide what they really wanted to do by opening the file and calling the load_hdf5() method (the load / save methods are also used when writing / reading the focalplane to a Group of a larger HDF5 file). However, this change does make things less convenient for loading data serially from a notebook for example. We could restore "filename" constructor argument, at the risk of it being misused in parallel workflows. Thoughts?

giuspugl commented 2 years ago

Thanks @keskitalo and @tskisner for reviewing this issue! I am fine with calling load_hdf5() as you 're recommending, as i agree this is less convenient and intuitive for serial runs from a notebook. However, I think that if the docstring encodes this detail (perhaps with some example for serial runs as @keskitalo did above) and it is documented then I don't see anything against adopting a unified approach. we will be surely safe by misuses in parallel workflows.

tskisner commented 2 years ago

Ok, good point. I will include an example in the docstring. Will leave this open until that is fixed.