UserCommands object is mutable and it is not copied when instantiating OpticalTrain

[joao-aveiro]

When instantiating the OpticalTrain class, if a UserCommands object is used, the OpticalTrain.cmds attribute will be set with the UserCommands object without a copy being made. As a results, changing a simulation parameter/commmand in one OpticalTrain object will lead to every other object of this class being modified too if they were instantiated with the same UserCommands object. To exemplify:

import scopesim as sim


if __name__ == "__main__":
    # Create sims and load mds
    # sim.download_package(["instruments/METIS", "telescopes/ELT", "locations/Armazones"])
    cmd = sim.UserCommands(use_instrument='METIS', set_modes=['img_lm'])
    metis1 = sim.OpticalTrain(cmd)
    metis2 = sim.OpticalTrain(cmd)

    # Testing
    print(f"\n Before:"
          f"\n\t - {metis1.cmds['!SIM.sub_pixel.flag']=}"
          f"\n\t - {metis2.cmds['!SIM.sub_pixel.flag']=}")

    print("\n Changing metis2 cmd: !SIM.sub_pixel.flag=True ")
    metis2.cmds["!SIM.sub_pixel.flag"] = True

    print(f"\n After:"
          f"\n\t - {metis1.cmds['!SIM.sub_pixel.flag']=}"
          f"\n\t - {metis2.cmds['!SIM.sub_pixel.flag']=}")

Output:

 Before:
	 - metis1.cmds['!SIM.sub_pixel.flag']=False
	 - metis2.cmds['!SIM.sub_pixel.flag']=False

 Changing metis2 cmd: !SIM.sub_pixel.flag=True 

 After:
	 - metis1.cmds['!SIM.sub_pixel.flag']=True
	 - metis2.cmds['!SIM.sub_pixel.flag']=True

I believe this shoulnd't happen and that every OpticalTrain obejct should be independent of eachother, even if they were instantiated with the same UserCommands object.

[joao-aveiro]

To fix this, I propose to make a deepcopy of the UserCommands object while instantiating the OpticalTrain class. I have submitted a proposal with this fix in PR #186.

[hugobuddel]

Thanks. This non-copying has been a frustration for many people. My main problem wast that if you do a .readout() in-between the two instantiations, that it is sometimes not possible to do a second instantiation / readout at all. So for user convenience it is important to fix this, thanks!

@astronomyk made me deputy ScopeSim maintainer. However, I don't feel comfortable merging #186 just now because of Chesterton’s Fence. That is, there might be a reason for this specific behavior that we're not aware of and that merging #186 would break something (and the CI fails since yesterday because of #184). I expect it was just an oversight though. Next week @astronomyk will be back, so we can probably merge it then.

Maybe it would also be time to add type annotations, thanks for updating the docstring.

[joao-aveiro]

I was also worried that this was done for a reason or that this fix could break something, but so far that doesn't seem to be the case. Nevertheless, I have only been using his package for the past few weeks, so I am far from proficient in it and I might be overlooking something, so I agree that feedback from other contributors is invaluable.

As for the type annotations, in other projects I have been using type hints and type checkers, such as mypy, extensively. Not wanting to impose on your stylistic decisions and/or your development roadmap, I have to say that such an improvement would probably make the package much more robust and more readable for both contributors or new users (like me).