adding instrumental model #221
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request introduces an instrumental model to the fitting process, allowing for the calibration of energy gain and shift in observations. It also includes minor fixes and improvements to integration and plotting functionalities. Sequence diagram for forward model with instrument calibrationsequenceDiagram
participant FM as Forward Model
participant IM as Instrument Model
participant M as Model
FM->>IM: get_gain_and_shift_model(obs_name)
alt observation is reference
IM-->>FM: None, None
else observation needs calibration
IM-->>FM: gain_func, shift_func
end
Note over FM: Apply energy shift if present
Note over FM: Apply gain correction if present
FM->>M: photon_flux(parameters, energies)
M-->>FM: flux
Note over FM: Apply transfer matrix
Class diagram for the new instrument model hierarchyclassDiagram
class InstrumentModel {
+reference_observation_name: str
+gain_model: GainModel
+shift_model: ShiftModel
+get_gain_and_shift_model(observation_name: str)
}
class GainModel {
<<abstract>>
+numpyro_model(observation_name: str)*
}
class ShiftModel {
<<abstract>>
+numpyro_model(observation_name: str)*
}
class ConstantGain {
+prior_distribution: Distribution
+numpyro_model(observation_name: str)
}
class PolynomialGain {
+prior_distribution: Distribution
+degree: int
+numpyro_model(observation_name: str)
}
class PolynomialShift {
+prior_distribution: Distribution
+degree: int
+numpyro_model(observation_name: str)
}
GainModel <|-- ConstantGain
GainModel <|-- PolynomialGain
ShiftModel <|-- PolynomialShift
InstrumentModel o-- GainModel
InstrumentModel o-- ShiftModel
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @renecotyfanboy - I've reviewed your changes - here's some feedback:
Overall Comments:
- Replace np.nanmin/np.nanmax calls with min/max or np.minimum/np.maximum when comparing scalar values (e.g. lowest_y and y_observed_bkg) to avoid parameter misinterpretation.
- Use the correct keyword (a_min) in jnp.clip to ensure consistency with the JAX API.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
|
|
||
| class InstrumentModel: | ||
| def __init__( |
There was a problem hiding this comment.
suggestion: Consider adding validation for gain and shift model combinations
Some combinations of gain and shift models might not be physically meaningful or could lead to numerical instabilities. Consider adding validation in the constructor.
Suggested implementation:
def __init__(
self,
reference_observation_name: str,
gain_model: GainModel | None = None,
shift_model: ShiftModel | None = None,
):
self.reference = reference_observation_name
# Validate gain and shift model combinations
if gain_model is not None and shift_model is not None:
# Check if both models reference the same observation
if (gain_model.reference != reference_observation_name or
shift_model.reference != reference_observation_name):
raise ValueError(
"When both gain and shift models are used, they must reference "
"the same observation as the instrument model."
)
# Add any physics-based validation specific to your use case
# For example, if certain polynomial orders shouldn't be combined
if isinstance(gain_model, PolynomialGainModel) and isinstance(shift_model, PolynomialShiftModel):
if gain_model.order > 1 and shift_model.order > 1:
raise ValueError(
"Using high-order polynomials for both gain and shift "
"simultaneously may lead to numerical instabilities."
)
self.gain_model = gain_model
self.shift_model = shift_modelThe implementation above assumes:
- GainModel and ShiftModel classes have a 'reference' attribute
- There are PolynomialGainModel and PolynomialShiftModel classes with an 'order' attribute
You may need to:
- Adjust the specific validation rules based on your physics requirements
- Add or modify the checks based on the actual model types you support
- Update the error messages to match your project's terminology
| Return the gain and shift models for the given observation. It should be called within a numpyro model. | ||
| """ | ||
|
|
||
| if observation_name == self.reference: |
There was a problem hiding this comment.
issue (code-quality): Remove unnecessary else after guard condition (remove-unnecessary-else)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add support for instrument models in spectral fitting.
New Features:
Tests:
📚 Documentation preview 📚: https://jaxspec--221.org.readthedocs.build/en/221/