Units Implimentation #23
Conversation
|
@astrofrog : You know best what exactly is going to happen to models in astropy, so please have a short look here. I'm wondering if there is any astropy effort that could be used here. |
|
|
||
| def test_basic(self): | ||
| self.fitter(self.model_micron, self.x, self.y) | ||
|
|
There was a problem hiding this comment.
Does it makes sense to actually run a fitter here, so you can look at the return value?
There was a problem hiding this comment.
Just trying to check that no exceptions are thrown at the moment.
I was planning extending the tests over this weekend to test the values make sense.
saba/main.py
Outdated
| @@ -362,6 +360,11 @@ def __call__(self, models, x, y, z=None, xbinsize=None, ybinsize=None, err=None, | |||
| """ | |||
|
|
|||
| tie_list = [] | |||
|
|
|||
| _models, _x, _y, _z, _xbinsize, _ybinsize, _err, _bkg = self.remove_units(models, x, y, z, xbinsize, ybinsize, err, bkg, equivalencies) | |||
There was a problem hiding this comment.
Where are these _XXX quantities used? I'm sure they are, but I cannot find the line where that happens.
There was a problem hiding this comment.
I though I had removed the _'s I'll sort that out :)
saba/main.py
Outdated
| @@ -439,6 +442,188 @@ def get_sampler(self, *args, **kwargs): | |||
| """ | |||
| return SherpaMCMC(self, *args, **kwargs) | |||
|
|
|||
| def remove_units(self, models, x, y, z=None, xbinsize=None, ybinsize=None, err=None, bkg=None, equivalencies=None): | |||
| """ | |||
| This stripts data of there units so they can be fit. | |||
saba/main.py
Outdated
| try: | ||
| n_data = x.ndim | ||
| if z is None: | ||
| assert x.ndim == y.ndim, AstropyUserWarning("x and y dimensions don't match") |
There was a problem hiding this comment.
Why not just raise a ValueError here? Is there a use case where we should just proceed with a warning?
|
I do not understand what's going on in this code. That's fine. I do not understand what's going on in the astropy PRs that add quantities to models either. |
|
@hamogu @astrofrog the new methods are based off the |
|
I've found a fairly major problem which comes up in the dataset creation brought about by the unit stripping code, so no point in looking at it till I fix that. @hamogu thanks for spotting that I hadn't removed the _'s |
|
Also @hamogu this is pretty much doing as you describe converting all the inputs and parameters into common units, running the fit on floats then adding the units back afterwards. |
So with the units being integrated into
astropy.modeling, I so implemented some stuff. This uses the PR#4855