sunpy / sunkit-spex

:construction: Under Development :construction: A package for solar X-ray spectroscopy
BSD 3-Clause "New" or "Revised" License
22 stars 26 forks source link

Rhessi srm editable (post rename) #143

Closed settwi closed 3 months ago

settwi commented 4 months ago

Addresses issue #116 by

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

codecov-commenter commented 4 months ago

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 ```diff @@ 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.

KriSun95 commented 3 months ago

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?

samaloney commented 3 months ago

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.

settwi commented 3 months ago

@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.

samaloney commented 3 months ago

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

settwi commented 3 months ago

ok. i'll work on it

samaloney commented 3 months ago

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.

settwi commented 3 months ago

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

settwi commented 3 months ago

ruh roh readthedox failed one second.

settwi commented 3 months ago

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

samaloney commented 3 months ago

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 😉