WIP: Update example to use spec object and units#163
WIP: Update example to use spec object and units#163samaloney wants to merge 36 commits intosunpy:mainfrom
Conversation
samaloney
force-pushed
the
docs-spec-object
branch
from
30e3a84 to
6dc46f5
Compare
jajmitchell
force-pushed
the
docs-spec-object
branch
from
49bb84d to
839b0cb
Compare
rebase rebase
Fixed fitting_simulated_data conflicts
Fix attempt 2 fitting_simulated_data
Compound fitting now works
Tidy and adds changelog
Apply unit handling and binning normalisation
Fixes tests
samaloney
force-pushed
the
docs-spec-object
branch
from
354fce2 to
42d9c1c
Compare
Fixes example and unit handling in models.py. Also ensures minimzer reads photon_edges, needs to be looked at however as they are averaged edges.
Adds correct astropy into pyproject.toml
Fixes dependencies
Fix Scipy optimising function to correctly use bin_edges and add name to nonthermal along with limits on parameters
Spectrum llows counts fluxes and photon bins
Fix pre-commit
Fixes pre-commit
Fixes nonthermal test
Update astropy tag
Fixes pyproject.toml
Fix spectrum indexing
Second attempt at fixing spectrum
Attempt 3 at fixing spectrum
Fixes optimising_functions
Second attempt fix optimising_functions
Fixes spectrum
settwi
left a comment
settwi
left a comment
There was a problem hiding this comment.
Main thing that i would like to see changed is removal of the comment blocks
This is also a big PR. could you add a bit to the conversation about what tests are in place that correspond to the changes? it's a bit hard to parse through everything having not worked on it
finally i think we should revisit the bin centers vs edges question. i think having the option to do both is confusing and could lead to strange errors for folks doing analysis down the road
There was a problem hiding this comment.
do we want to keep this file? it seems like it's just wrapping a function call. might be more flexible to just not have it
| # print(x) | ||
| # print(input_widths) | ||
| # print(self.matrix) | ||
| # print(c) | ||
| # print(output_widths) | ||
|
|
||
| # print(self.input_axis.size) | ||
| # print(self.output_axis.size) | ||
| # print(x.size) | ||
| # print(input_widths.size) | ||
| # print(self.matrix.size) | ||
| # print(c.size) | ||
| # print(output_widths.size) |
There was a problem hiding this comment.
for merging code we should delete chunks that are commented out
| if bin_specification == "edges": | ||
| obj._bin_edges = bin_edges | ||
| elif bin_specification == "centers": | ||
| obj._bin_edges = None |
There was a problem hiding this comment.
i think we should drop support for bin centers. for X-ray data, we only ever work with bins. for data that is not X-ray, we will need an entirely different format anyway.
| _input_units={"x": u.dimensionless_unscaled}, | ||
| _output_units={"y": u.dimensionless_unscaled}, |
There was a problem hiding this comment.
user-facing variables should not have underscores in front of them if they are supposed to be used. are these going to be commonly used? should they be given defaults instead?
|
@settwi , thanks for the comments. I will take a proper look through and make some changes later in the week, there are some things that definitely need changing and I will add some more description of all the changes that I have made. I am giving the workshop tomorrow and so need this branch to remain the same until after tomorrow |
|
@jajmitchell what is the status of this branch? |
|
@settwi I think that its best that this PR is broken down into a few smaller PRs with more defined scope, particularly as much will need to be changed, due to the decisions made regarding the spectrum object and fitter etc |
3 participants
PR Description
Update exiting fitting demo to use units and work with astropy modelling.