sunkit-spex icon indicating copy to clipboard operation
sunkit-spex copied to clipboard

Rhessi srm editable (post rename)

Open settwi opened this issue 1 year ago • 2 comments

Addresses issue #116 by

  • auto-picking the correct RHESSI SRM based on the selected time range of the data.
  • making it easy to add systematic error e.g.: sunkit_fitter.systematic_error = 0.1

Also deletes the STIX loader. I think I did that in the past when I was frustrated with how it (didn't really) work. We should probably add that back before this gets merged... if it ever does :p

This change makes it actually possible to do good analysis on RHESSI data. I am going to use this branch in a paper so it would be great if we could get the changes merged in.

- WS

settwi avatar May 16 '24 20:05 settwi

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Please upload report for BASE (main@eb860e9). Learn more about missing BASE report.

Files Patch % Lines
sunkit_spex/fitting_legacy/instruments.py 50.00% 1 Missing :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #143   +/-   ##
=======================================
  Coverage        ?   61.21%           
=======================================
  Files           ?       18           
  Lines           ?     2741           
  Branches        ?        0           
=======================================
  Hits            ?     1678           
  Misses          ?     1063           
  Partials        ?        0           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar May 16 '24 20:05 codecov-commenter

This PR is looking good. Would it be worth while creating the extern module in fitting_legacy and moving the RHESSI code there in this PR as well?

KriSun95 avatar May 30 '24 21:05 KriSun95

This PR is looking good. Would it be worth while creating the extern module in fitting_legacy and moving the RHESSI code there in this PR as well?

The extern module is already there just need to move the code and maybe convert the ipynb to py for easier diffs or at least strip out the cell outputs.

samaloney avatar May 31 '24 08:05 samaloney

@DanRyanIrish @samaloney @KriSun95

pre-comit.ci is failing because autopep8, etc. are failing on poorly-formatted code in lots of files. i don't really want to touch them in this commit.

settwi avatar Jun 13 '24 16:06 settwi

I think you'll either need to rebase or merge in main as theses have all been fixed and pre-commit passes on main.

samaloney avatar Jun 13 '24 16:06 samaloney

ok. i'll work on it

settwi avatar Jun 13 '24 16:06 settwi

It looks like pre-commit was able to fix all the issues so if you run pre-commit run -a double check the diff and then add the changes should be good to go.

samaloney avatar Jun 13 '24 16:06 samaloney

oh wow, i didn't know you could do that :p

settwi avatar Jun 13 '24 16:06 settwi

ruh roh readthedox failed one second.

settwi avatar Jun 13 '24 16:06 settwi

I’m pretty sure they aren’t valid because i condensed the code. but we could add some version back if need be.

settwi avatar Jun 13 '24 17:06 settwi

I think it would be good but as this is't in the main package I'm not going to hold it up over that but I would strongly encourage you too add some tests at some point 😉

samaloney avatar Jun 13 '24 17:06 samaloney