Add fitting light curve #505
Conversation
for more information, see https://pre-commit.ci
# Conflicts: # ztf_viewer/model_fit.py
for more information, see https://pre-commit.ci
# Conflicts: # ztf_viewer/model_fit.py
for more information, see https://pre-commit.ci
# Conflicts: # ztf_viewer/pages/viewer.py
hombit
left a comment
hombit
left a comment
There was a problem hiding this comment.
Pre-approve it so deployment would run
There was a problem hiding this comment.
Could you please update this file from the master branch?
ztf_viewer/model_fit.py
Outdated
|
|
||
|
|
||
| class ModelFit: | ||
| base_url = "http://host.docker.internal:8000/api/v1" |
There was a problem hiding this comment.
This should be a configurable URL
ztf_viewer/model_fit.py
Outdated
| ], | ||
| "ebv": ebv, | ||
| "name_model": fit_model, | ||
| "redshift": [0.05, 0.3], |
There was a problem hiding this comment.
Sounds too arbitrary, should be at least class-level attribute, at most a parameter.
There was a problem hiding this comment.
I added Target class
ztf_viewer/model_fit.py
Outdated
| json={ | ||
| "parameters": params, | ||
| "name_model": name_model, | ||
| "zp": 8.9, |
ztf_viewer/model_fit.py
Outdated
| return params | ||
|
|
||
| def get_curve(self, df, dr, ref_mag_values, bright, params, name_model): | ||
| path = "/sncosmo/get_curve" |
There was a problem hiding this comment.
Should be a class/object attribute
There was a problem hiding this comment.
I added a path in ModelFit and a function to set it.
ztf_viewer/model_fit.py
Outdated
| self._api_session = requests.Session() | ||
|
|
||
| def fit(self, df, fit_model, ref_mag_values, dr, ebv): | ||
| path = "/sncosmo/fit" |
There was a problem hiding this comment.
Should be a class/object attribute
ztf_viewer/model_fit.py
Outdated
| if not ref_mag_values: | ||
| oid_ref = {} | ||
| try: |
There was a problem hiding this comment.
I don't really understand this part of the code.
- What
ref_mag_valuessupposed to be? True/False? Or None/dict? If the least, than you never use that dict, and also please change the statement to more explicit:if ref_mag_values is not None. - If the catalog not available, you
passand never use the references. It is not a good behavior, because the result will change without user's consent, and do it implicitly without any notification. Maybe it is better to just fail, than do that - You are mutating the input dataframe. It is not a good practice, what if the caller would reuse it? Please do something like
df = df.copy()if you want to change the values.
There was a problem hiding this comment.
I removed ref_mag_values and took into account the unavailability of getting a ref, and now I'm not changing the input dataframe.
ztf_viewer/model_fit.py
Outdated
| ] | ||
| except (NotFound, CatalogUnavailable): | ||
| pass | ||
| res_fit = requests.post( |
There was a problem hiding this comment.
What is the expected behavior if it fails? I think we normally catch it and raise a CatalogUnavailable error, which can be processed in the UI code.
There was a problem hiding this comment.
I took into account the unavailability of getting a ref
ztf_viewer/pages/viewer.py
Outdated
| else: | ||
| raise ValueError(f"{lc_type = } is unknown") | ||
| if name_model and fit_params: | ||
| df_fit = model_fit.get_curve(df, dr, ref_mag_values, bright, json.loads(fit_params), name_model) |
There was a problem hiding this comment.
What if it fails? We would lose the whole plot I guess...
for more information, see https://pre-commit.ci
2 participants
No description provided.