Skip to content

Conversation

@Mateasek
Copy link
Member

@Mateasek Mateasek commented Jun 5, 2025

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.

Mateasek added 3 commits June 5, 2025 16:16
This should add the possibility to fully investigate what emission
models an observation was performed with.
@Mateasek Mateasek requested a review from Copilot October 16, 2025 20:12
Copy link

Copilot AI left a 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 line and lineshape attributes across emission model classes
  • Updated documentation to include the newly accessible attributes as :ivar entries
  • 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.

@jacklovell
Copy link
Member

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.

@Mateasek
Copy link
Member Author

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:

  1. I remove the tests which require atomic data, which are most of them
  2. Atomic data are obtained before tests are initiated, but I have no idea if it is a good idea or even possible

@jacklovell
Copy link
Member

Add some mock atomic data: doesn't have to be physically meaningful but just the bare minimum for the tests.

@Mateasek
Copy link
Member Author

good idea @jacklovell

@Mateasek
Copy link
Member Author

Mateasek commented Nov 2, 2025

I patched the file reading, the tests now pass. I think this is ready to go @jacklovell

@jacklovell jacklovell merged commit 36fde32 into cherab:development Nov 3, 2025
6 of 8 checks passed
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