dysmalpy / dysmalpy

Dynamical Simulation and Modeling Algorithm
Other
7 stars 1 forks source link

Incorrect default aperture in Model_set._model_aperture_r() #130

Open nestoramit opened 1 week ago

nestoramit commented 1 week ago

The current implementation assumes it always has a 'disk+bulge' mass component to get Reff

Suggests to use the same thing we had in other cases, to check if either a disk+bulge, disk or ring are included:

if 'disk+bulge' in self.mass_components:
    if self.mass_components['disk+bulge']:
        r_ap = self.components['disk+bulge'].r_eff_disk.value
elif 'disk' in self.mass_components:
    if self.mass_components['disk']:
        r_ap = self.components['disk'].r_eff.value
elif 'ring' in self.mass_components:
    if self.mass_components['ring']:
        r_ap = self.components['ring'].ring_reff()
else:
    r_ap = 1.

file: models/model-set.py line: 193

sedonaprice commented 1 week ago

Hi Amit --- in general, please note the specific file + function (even best with line numbers) for bug reports + issues!

sedonaprice commented 1 week ago

One logical option would be to make model_key_re an attribute that is set in a ModelSet as part of assembling the model components / with an override by setting it explicitly.

Possibility:
have a ModelSet.model_key_re value set by default upon loading new components, going through an order of pre-determined preferences (disk+bulge > disk > ring > other default). BUT, if model_key_re is set by the user explicitly (and not with this auto-setter), don't override when adding later keys. So: have a getter+setter for model_key_re. If setter is called with a value input, set a "don't override" hidden key. Otherwise run the auto script.

eg, see Galaxy.z in dysmalpy/galaxy.py: if set by an explicit call within the @model_key_re.setter framework, set ModelSet._model_key_re_autoset = False. Default ModelSet._model_key_re_autoset = True, and ModelSet._model_key_re=None. Then when adding components, if ModelSet._model_key_re_autoset = True, directly write to ModelSet._model_key_re using the auto choice script

This way, we would avoid having to pass this as a kwarg in other calls (which was missed in the current implementation)

nestoramit commented 1 week ago

Sorry! I added the file and line number to the original post.

that sounds good, and that way it could be used for all places where we call for fdm(reff) to be calculated for whatever mass components connected to the model. From what I remember it also happens at the tied functions:

both of them could call ModelSet.get_dm_frac_r_ap() to calculate fdm for the default Reff, which would be defined as you just laid out.