Kris's Fitting Infrastructure #56
Conversation
Codecov Report
@@ Coverage Diff @@
## master #56 +/- ##
===========================================
- Coverage 93.45% 29.59% -63.86%
===========================================
Files 5 18 +13
Lines 626 3284 +2658
===========================================
+ Hits 585 972 +387
- Misses 41 2312 +2271
Continue to review full report at Codecov.
|
|
Thanks so much for this PR @KriSun95! A huge effort! |
sunxspex/sunxspex_fitting/io.py
Outdated
| Returns | ||
| ------- | ||
| The channel numbers, counts, and the livetime for the observation. | ||
| """ |
There was a problem hiding this comment.
Great work on your docstrings!
sunxspex/__init__.py
Outdated
| @@ -1,4 +1,5 @@ | |||
| # Licensed under a 3-clause BSD style license - see LICENSE.rst | |||
|
|
|||
| from .version import __version__ | |||
| #from .version import __version__ | |||
There was a problem hiding this comment.
| #from .version import __version__ |
sunxspex/emission.py
Outdated
|
|
||
| # Max number of points | ||
| maxfcn = 2048 | ||
|
|
sunxspex/sun_xspec.py
Outdated
| """ | ||
| import numpy as np | ||
| import pandas as pd | ||
| #from scipy.special import lpmv |
There was a problem hiding this comment.
| #from scipy.special import lpmv |
sunxspex/sun_xspec.py
Outdated
| #import astropy.constants as c | ||
| #from quadpy.c1 import gauss_legendre |
There was a problem hiding this comment.
| #import astropy.constants as c | |
| #from quadpy.c1 import gauss_legendre |
sunxspex/sun_xspec.py
Outdated
| logging.basicConfig(filename='xspec.log', level=logging.DEBUG) #for now this helps catch Python errors that are not printed to the terminal by XSPEC | ||
|
|
||
| # Central constant management | ||
| global const #whyyyyyy there has got to be a better way to do this |
There was a problem hiding this comment.
Why is global being used here?
|
@KriSun95 I think it would be educational to read the Python style guide, PEP8. Most code in the sunpy ecosystem, as well as the broader scientific Python stack follows this guide. If you fold these guidelines into your own writing style, it will make reading other people code easier and more enjoyable as it will be close to your own style. We should also try running a codestyle test locally that will automatically improve the readability of your code. Let's discuss that on a call. |
There was a problem hiding this comment.
First thing amazing work @KriSun95 and also @elastufka.
I've done a quick initial review of most of the code and have left a few specific comments and questions and I have some more general points below. Please note you are not expected to fixed/address these issues and you can also disagree this should all be a discussion.
- codes style and formatting looks very good probably just need to enable some more checks e.g. large files and isort, maybe black code formatter?
- doc strings all look good but we need to add to the built docs and check how the look in html
- tests we need tests for all code ideally, initially could focus on everything excluding plotting and then later take care of plotting too.
- As discussed during the meeting I think we need to review the use cases and maybe improve the API maybe separate
Spectrum,FitterandErrorclasses which are compose into a parent objectsSpectrumModelandSpectrumModelCollectionbut this needs to be fleshed out much more carefully - Some of the classes are very big it shouldn't be to hard to move some of the functionality to new file to keep them a bit more manageable
| @@ -0,0 +1,30 @@ | |||
| 1.00E+00 | |||
There was a problem hiding this comment.
minimal header with units would be idea but if this file is taken from somewhere else then it ok
There was a problem hiding this comment.
this file can be removed, no need for it now that it's an option in XSPEC
sunxspex/__init__.py
Outdated
| @@ -1,4 +1,5 @@ | |||
| # Licensed under a 3-clause BSD style license - see LICENSE.rst | |||
|
|
|||
| from .version import __version__ | |||
| #from .version import __version__ | |||
There was a problem hiding this comment.
what was the reason for this?
There was a problem hiding this comment.
don't know about Kris but this raised an error for me on import sunxspex
There was a problem hiding this comment.
This caused the same error for me too
sunxspex/sun_xspec.py
Outdated
| """ | ||
| import numpy as np | ||
| import pandas as pd | ||
| #from scipy.special import lpmv |
There was a problem hiding this comment.
So few general code style things here remove unused import and short by python, then external lib and then sunxspec we should probably just set up isort as part of our ci
So e.g.
import logging
from datetime import datetime as dt
import numpy as np
import pandas as pd
from astropy import units as u
from sunxspex import thermal
from sunxspex import constants
from sunxspex.emission import split_and_integrate, split_and_integrate0
sunxspex/sun_xspec.py
Outdated
| flux[:]=[internal_flux[j] if j in i and j!=i[-1] else prev for j,prev in enumerate(flux)] | ||
| #logging.info(f"{flux[:20]}") | ||
|
|
||
| class ThickTargetModel(XspecModel): |
There was a problem hiding this comment.
btw I will make a new PR, this code does not integrate over energy bins and is therefore incorrect
no guarantee that I will write a legible docstring anytime soon though 😈
sunxspex/sun_xspec.py
Outdated
| raise NotImplementedError | ||
|
|
||
|
|
||
| class ThickTargetModel0(XspecModel): #original integration for comparison |
| # Get a default class for the instrument specfic loaders | ||
| # Once the instrument specific loaders inherit from this then all they really have to do is get the spectral | ||
| # data they want to fit in the correct dictionary form and assigned to `self._loaded_spec_data`. | ||
| class InstrumentBlueprint: |
There was a problem hiding this comment.
maybe an abstract class?
There was a problem hiding this comment.
Definitely need some restructuring here from the previous discussions.
| bounds = dict(zip(self.param_names, param_bounds)) | ||
| errors = dict(zip(self.param_names, param_errors)) | ||
| # create table | ||
| self.parameter_info = pd.DataFrame(dict(zip(self.states, [status, values, bounds, errors]))) |
There was a problem hiding this comment.
If we do go units would astropy.table.QTable be worth it?
There was a problem hiding this comment.
I don't know if I have ever used a QTable so I'm not sure. Depending on where units are implemented, more work would need done one handling these in the fitting code and how the model functions are to be created. Definitely worth a serious discussion.
| if _for=="frozen": | ||
| return ["frozen", "freeze", "chill", "fix", "fixed", "secure", "stick", "glue", "preserve", "restrain", "restrained", "cannot_move", "cant_move", "canny_move", "married"] | ||
| elif _for=="free": | ||
| return ["free", "thaw", "loose", "unrestrained", "release", "released", "can_move", "single"] | ||
| elif _for=="tie": | ||
| return ["tie", "tied", "bind", "tether", "join", "joined", "in_a_relationship_with"] |
There was a problem hiding this comment.
I wonder if less is more here in only supporting a few keywords?
There was a problem hiding this comment.
Probably less. This mainly came from my inability to remember to type "freeze" and "thaw" when I was testing out the code (to be consistent with XSPEC) where I kept writing "fix" or "free". A single method for each status might even be more useful (I think Dan suggested this)? Just need to decide on which word to be consistent with for each status value.
| ### Issue when using np.float64 numbers for the parameters as it ends up returning all nans and infs but rounding to 15 decimal places fixes this?????? | ||
|
|
||
| # The defined models shouldn't have duplicate parameter input names | ||
| defined_photon_models = {"f_vth":["T", "EM"], |
There was a problem hiding this comment.
maybe we can put some of this in to the models, so can be called without these wrappers?
There was a problem hiding this comment.
Definitely happy to do that. These wrappers are here for a few reasons but mainly to have the inputs in the form I chose for the fitting code and to get them in one place but they can obviously be changed and made better.
sunxspex/thermal.py
Outdated
| # from scipy.interpolate import interp1d | ||
| # # load in everything for the chianti_kev_cont code of "mine". This only needs done once so do it here. | ||
|
|
||
| # def chianti_kev_units(spectrum, funits, wedg, kev=False, earth=False, date=None): |
There was a problem hiding this comment.
pick, merge etc so only have one or both if they do different things?
There was a problem hiding this comment.
My aim was to try and get this to work and then I have clearly forgot about it. I think this is something Dan and I might look at to combine both versions of this but I can remove it for this PR and update the continuum calculation further down the road.
|
pre-commit.ci autofix |
PR Description
Includes fitting infrastructure code.
PR Checklist
pytestpasses).Fixes #XXXX -orCloses #XXXX -comment to auto-close the issue that your PR addresses