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

Restructure repo. #151

Closed DanRyanIrish closed 4 months ago

DanRyanIrish commented 4 months ago

PR Description

This PR restructures the repo in line with the planned refactor. It moves the fitting_legacy subpackage and all model-related modules to a new legacy subpackage and creates a new models subpackage. Currently, this holds a few empty subpackages and modules to show how it should/could be structured going forward.

I suggest that we use this structure for our refactor, starting with the following:

There would be more to do, but this would actually prove the fundamental pillars of our refactor.

Other crucial work involves refactoring the non-thermal functions and making them models.

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 42.88618% with 281 lines in your changes missing coverage. Please review.

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

Files Patch % Lines
sunkit_spex/models/physical/thermal.py 25.88% 189 Missing :warning:
...unkit_spex/legacy/fitting_legacy/rhes_spec_code.py 14.54% 47 Missing :warning:
...unkit_spex/legacy/fitting_legacy/stix_spec_code.py 22.50% 31 Missing :warning:
sunkit_spex/models/physical/io.py 88.42% 14 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #151 +/- ## ======================================= Coverage ? 49.89% ======================================= Files ? 25 Lines ? 3403 Branches ? 0 ======================================= Hits ? 1698 Misses ? 1705 Partials ? 0 ```

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

DanRyanIrish commented 4 months ago

ping @samaloney

DanRyanIrish commented 4 months ago

@KriSun95 If you're happy with this, can you officially approve it?

DanRyanIrish commented 4 months ago

@samaloney It would be good to get your approval here. The main things you should know about the new structure are:

Mainly the names and the structure is what I'd like your views on before merging. E.g., perhaps math.py should be mathematical.py so the path is models.mathematical.powerlaw? More modules can be added in the future if needed of course, e.g. models.integrate for the integrate module now living in legacy.

samaloney commented 4 months ago

Over all I think this sounds good. I was just looking at some other existing packages and how they layout their models and some discussion we had before in #42.

Questions:

  1. models or modeling don't really mind
  2. Why move the io inside the model?

I think the integration stuff could live in it own module as it is essentially generic numerical integration.

My suggestion would be the following as I do like the idea of separating physically meaningful models from other mathematical or functionals ones.

.
└── models
    ├── models.py
    ├── instrument_response.py # don't need this now as Matrix would do
    └── physical.py  #these could be a directories/submodules if needed as you have 

but the imports could be like this (updating the case for class v function)

from sunkit_spex.models.physical import thermal, thin_target, thick_target, warm_target
from sunlit_spex.models import Matrix
from sunkit_spex.models import Powerlaw

or if we find we have a lot of powerlaw or non_thermal models in the future we can change to

from sunkit_spex.models.powerlaw import MyFavPowerlaw
from sunkit_spex.models.pyhsical.nonthermal import WarmTarget, MediumHotTarget 😆 

Also moving stuff around the namespace while annoying is possible so we aren't going to be stuck if we find we need to change we can so it really doesn't matter too much so going to approve this PR.

DanRyanIrish commented 4 months ago
1. `models` or `modeling` don't really mind

OK. Let's stick with models then, since there's no clear convention.

2. Why move the io inside the model?

The thermal io.py was specifically for reading in CHIANTI tables for the thermal model. There's another io in fitting_legacy. There could be an io module for reading spectral files as well. I agree this is messy. But perhaps changing the name from io or moving it out later is an option too.

.
└── models
    ├── models.py
    ├── instrument_response.py # don't need this now as Matrix would do
    └── physical.py  #these could be a directories/submodules if needed as you have 

OK. I'll change photon to physical. What would go in models.py? Would this be equivalent to mathematical.py, i.e. PowerLaw?

or if we find we have a lot of powerlaw or non_thermal models in the future we can change to

from sunkit_spex.models.powerlaw import MyFavPowerlaw
from sunkit_spex.models.pyhsical.nonthermal import WarmTarget, MediumHotTarget 😆 

Even if there's a sunkit_spex.models.physical.nonthermal subpackage/module, we can use the physical.__init__ to enable importing directly from sunkit_spex.models.physical.

DanRyanIrish commented 4 months ago

Ah. Do you mean that models.py would hold things like PowerLaw and MatrixModel? As in generic, non-physical, non-instrumental models?

samaloney commented 4 months ago

Ah. Do you mean that models.py would hold things like PowerLaw and MatrixModel? As in generic, non-physical, non-instrumental models?

Yea exactly anything that's not a physical model - if we find we have a too many we could create additional submodules/namespaces response, or powerlaw down the line.

DanRyanIrish commented 4 months ago

OK, great. I've made those changes. Pending tests passing, can you approve this?

settwi commented 4 months ago

why is the thermal code duplicated in legacy and models? @DanRyanIrish

settwi commented 4 months ago

from Element:

why is the stix and rhessi “spec” code back in legacy? it’s kind of defunct now that we moved rhessi to “extern”