LukeBarnard
commented
4 years ago Thanks David!
* Where you define the inner boundary condition, it might be nice to do a quick plot of it to get a visual handle on it.
Yes, I agree, done.
All of the inputs here could, and probably should be quantity inputs (as opposed to just numbers). Astropy has a
quantity_inputdecorator that automatically checks units: see here for more info and examples.
Yes, I agree. I've updated the classes so that the input arguments should be quantities. I'd not heard of the quantity_input decorator before, so thanks for the heads up.
I think that solving with no CMEs is a common enough use case that setting
cme_list=[]by default might be a good option here.
I agree, done.
A lot of these could be quantities instead of just arrays, which would save you having to specify their output units (and prevent the user from having to look up the docstring to know what units are being outputted)
Most of these (all the coordinate and solution arrays at least) are quantities. I listed the units in the docstring just to try and make it clear what units should be expected. Do you think these would be better off removed from the docstring then?
There's a line in the jupyter example that is
t_launch = 0.5*u.day.to(u.s)- if astropy units are all working fine internally you shouldn't need the.to(u.s)bit here.Agreed and changed.
Again, these should all be quantity inputs (which would have the advantage a user could pass
np.pi * u.rador180 * u.degand get the same result).Yup, I agree. Done.
It would be nicer to remove
savehere, and have a separatedef save(self, tag='')method that does the saving. Means an extra line of code for the user, but also means if they run a model and then retrospectively want to save it they can.Yup, I agree, the save method was already in there for saving outside of solving.
Setting the over colour to white makes it look like data is missing instead of just has large values which are clipped, maybe setting it to a light grey would be better.
I agree, and have changed it to a light grey.
Hello, I've had a look through the code, and overall it looks really good! The example in the notebook is intuitive, and I understand what is going on all the time just from the plots and well named variables/methods/classes.
I've put a few suggestions below, which are based on looking through the Jupyter notebook example, and suggesting improvements to the user facing API. I've also had a quick look through the underlying code, and it looks sensible at a glance.
https://github.com/University-of-Reading-Space-Science/HUXt/blob/2d7b9b668d3414b003f5f3d533ec3037841b9bed/code/HUXt.py#L245 All of the inputs here could, and probably should be quantity inputs (as opposed to just numbers). Astropy has a
quantity_inputdecorator that automatically checks units: see here for more info and examples.https://github.com/University-of-Reading-Space-Science/HUXt/blob/2d7b9b668d3414b003f5f3d533ec3037841b9bed/code/HUXt.py#L319
I think that solving with no CMEs is a common enough use case that setting
cme_list=[]by default might be a good option here.https://github.com/University-of-Reading-Space-Science/HUXt/blob/2d7b9b668d3414b003f5f3d533ec3037841b9bed/code/HUXt.py#L218-L242
A lot of these could be quantities instead of just arrays, which would save you having to specify their output units (and prevent the user from having to look up the docstring to know what units are being outputted)
There's a line in the jupyter example that is
t_launch = 0.5*u.day.to(u.s)- if astropy units are all working fine internally you shouldn't need the.to(u.s)bit here.https://github.com/University-of-Reading-Space-Science/HUXt/blob/2d7b9b668d3414b003f5f3d533ec3037841b9bed/code/HUXt.py#L35
Again, these should all be quantity inputs (which would have the advantage a user could pass
np.pi * u.rador180 * u.degand get the same result).https://github.com/University-of-Reading-Space-Science/HUXt/blob/2d7b9b668d3414b003f5f3d533ec3037841b9bed/code/HUXt.py#L319
It would be nicer to remove
savehere, and have a separatedef save(self, tag='')method that does the saving. Means an extra line of code for the user, but also means if they run a model and then retrospectively want to save it they can.https://github.com/University-of-Reading-Space-Science/HUXt/blob/2d7b9b668d3414b003f5f3d533ec3037841b9bed/code/HUXt.py#L927
Setting the over colour to white makes it look like data is missing instead of just has large values which are clipped, maybe setting it to a light grey would be better.