-
Notifications
You must be signed in to change notification settings - Fork 24
Feature/model attr access #483
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/model attr access #483
Conversation
This should add the possibility to fully investigate what emission models an observation was performed with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds readonly property access to line and lineshape attributes of plasma emission models to enable better introspection and logging capabilities. The changes expose internal model attributes through public properties, allowing users to browse and verify what models were used in observations.
- Added property accessors for
lineandlineshapeattributes across emission model classes - Updated documentation to include the newly accessible attributes as
:ivarentries - Added changelog entry documenting the API enhancement
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cherab/core/model/plasma/total_radiated_power.pyx | Added property accessors for element and charge attributes |
| cherab/core/model/plasma/thermal_cx.pyx | Added property accessors for line and lineshape attributes |
| cherab/core/model/plasma/recombination.pyx | Added property accessors for line and lineshape attributes |
| cherab/core/model/plasma/impact_excitation.pyx | Added property accessors for line and lineshape attributes |
| cherab/core/atomic/line.pyx | Updated documentation to include attribute descriptions as :ivar entries |
| CHANGELOG.md | Added entry documenting the new emission model attribute access feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
This looks fine to me. Can you add some simple unit tests to check these attribute accesses? That'll guard against regressions in the future. Then I'm happy to merge. |
Kindo of following DRY in docstrings...
|
Thanks for the review @jacklovell I added tests for plasma models, but the problem is tests fail because the atomic data are not available. There are two options:
|
|
Add some mock atomic data: doesn't have to be physically meaningful but just the bare minimum for the tests. |
|
good idea @jacklovell |
|
I patched the file reading, the tests now pass. I think this is ready to go @jacklovell |
Added property access (readonly) to the line and lineshape attributes of emission models, related to #294 . This should allow fully browsing plasma emission models to verify what models were used in observation. This is good for taking logs and result serialisation.