b-thorne / PySM_public

PySM: Software for simulating the Galactic microwave sky
24 stars 29 forks source link

Pysm v3 dev #26

Closed b-thorne closed 4 years ago

b-thorne commented 6 years ago

@zonca Requiring units to be assigned by the user means we have to check the input units. This seems like it should be done via properties, but will cause a proliferation of code as there will be up to 10 properties with units for each model. Is this a reasonable thing to do?

zonca commented 6 years ago

No properties! I think we can try with this: http://docs.astropy.org/en/stable/units/quantity.html#functions-that-accept-quantities

On Mon, Aug 13, 2018, 11:32 Ben Thorne notifications@github.com wrote:

@zonca https://github.com/zonca Requiring units to be assigned by the user means we have to check the input units. This seems like it should be done via properties, but will cause a proliferation of code as there will be up to 10 properties with units for each model. Is this a reasonable thing to do?

You can view, comment on, or merge this pull request online at:

https://github.com/bthorne93/PySM_public/pull/26 Commit Summary

  • Possible implementation of unit checking.
  • Removed typo.
  • Fixed typo.

File Changes

Patch Links:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bthorne93/PySM_public/pull/26, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXYcmxBUUiIY2K76W_EQDTzfmJgvKGRks5uQfBngaJpZM4V7Ywo .

b-thorne commented 6 years ago

@zonca I have been applying that decorator to other functions, however the map arguments in the __init__ are to be Path objects. The reading is done within the __init__, so @units.quantity_input won't catch them when assigning self.map_I = hp.read_map(map_I).

zonca commented 6 years ago

@bthorne93 the most elegant way would be to read the units from the header of the input FITS files and automatically attach them. Does most of your templates have units already? If most of them have it, the best way would be to try to access them and raise an error if they do not. And add units in the headers of the files with missing units. Do you think this is reasonable?

b-thorne commented 6 years ago

@zonca I think that is reasonable, since users have to define the units at some point, and fits files should in general have units. From my experience there will be ambiguous cases of fits files with units K, and that is ambiguous for the code.

I will therefore define my own astropy unit, K_CMB, and its equivalency functions for K_RJ and MJysr. Then when reading in the maps the code will insist that the fits header has a units keyword for each field, and that the value be one of K_RJ, K_CMB, or MJysr.

zonca commented 6 years ago

Nice! I think it would also be useful to send a PR to astropy with those units, or at least ask them if they would accept such PR. But better first finalize the implementation for PySM

b-thorne commented 6 years ago

@zonca I have some more updates, but they are not ready to add to the pull request yet. Is the current state enough to start thinking about the MPI integration?

zonca commented 6 years ago

Yes, I'll start with what is already in the repo

zonca commented 4 years ago

development moved to https://github.com/healpy/pysm