Skip to content

Comments

WIP: Structure Docs#252

Draft
KriSun95 wants to merge 2 commits intosunpy:mainfrom
KriSun95:docs-infrastructure
Draft

WIP: Structure Docs#252
KriSun95 wants to merge 2 commits intosunpy:mainfrom
KriSun95:docs-infrastructure

Conversation

@KriSun95
Copy link
Collaborator

Trying to start adding to the docs describing the structure we're designing Sunkit-spex around. Comments and discussion welcome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm confused, not seeing the difference / need for instrument data container and spectrum container. What would be helpful here is a concrete example of something that requires the additional container?

To me if there is something instrument specific it doesn't belong in sunkit-spex. Any selection, re-binning etc of the spectral data (response or anything else) should be done on the Spectrum, SpectrumSequence, or SpectrumCollection objects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLDR; I believe at least, we would be far better off with two objects: a fancy one for the user or instrument team to put all their instrument data in; and one that is completely specialised exactly for what the code and fitting needs (produced from the object containing all the instrument data).

Below is quite long but I think there are a bunch of good reasons for this and I've tried to lay them out as best I can. Happy for rebuttals or other comments. I want to be on the same page as everyone here.


  1. The instrument container has to be instrument specific.

    • The API will be common, e.g. they'll all have a re-bin method or ways to define what values/dimensions they want their data to be fitted in, but how that's implemented will have to be different.
  2. The fitting doesn't care about all the instrument and observation data.

    • Some operations like re-binning during the fitting process (I'm not sure if we want/need to support this) would likely need all the data so it can be done properly for the given instrument, but for simple slicing to fit an energy range and all of the fitting only needs data X/Y and the model converting values (e.g., SRM, distance, etc.).
  3. The most fundamental case is a single 1D spectrum or a simple collection of them.

    • The way I've seen it, we can do everything we want with a 1D X and Y spectrum (or simple collection of them) without assuming any connection between them, anything beyond that is a special case. There wouldn't be an issue if we have different Xs in the different spectra for example which is really common in a lot of analysis I've been doing at least.
  4. Keeping things separate.

    • Another point @jajmitchell and I talked about was that it keeps exactly what is needed for fitting as simple and well defined as possible and any needed changes to the container of the instrument data doesn't affect that. It came down to the fact that any changes to what's needed for the fitting will cause a change in what the full data containers do but that's not the case going the other way.

All this being said, the 1D spectrum container is only really for our internal uses because we need something solid designed for fitting, it's not really for the user so it could be hidden from them but still accessible in a way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So potentially we could keep the high level information in the root but more detailed stuffs should go under discussion e.g. discussion/design discussion/fitting discussion/statistics if we're going to follow https://diataxis.fr

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes more sense and absolutely happy to move to it from what I have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants