Skip to content
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

IMAX effect integration #98

Merged
merged 6 commits into from
Mar 8, 2024
Merged

IMAX effect integration #98

merged 6 commits into from
Mar 8, 2024

Conversation

lowderchris
Copy link
Collaborator

@lowderchris lowderchris commented Feb 13, 2024

I've migrated some of the IMAX effect code from punchbowl to here in solpolpy directly. A punchbowl branch exists utilizing these changes, which can be pulled in once this pull request is resolved. Definitely let me know of any changes I can make here to improve. See any TODOs for outstanding questions I can wrap up.

  • mark imax effect as done in readme.md

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

Attention: Patch coverage is 88.57143% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 93.13%. Comparing base (be001f8) to head (5fb2eb1).
Report is 14 commits behind head on develop.

Files Patch % Lines
solpolpy/core.py 88.57% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #98      +/-   ##
===========================================
- Coverage    93.51%   93.13%   -0.38%     
===========================================
  Files            9        9              
  Lines          447      481      +34     
===========================================
+ Hits           418      448      +30     
- Misses          29       33       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jmbhughes
Copy link
Member

@s0larish could you take a look at the TODOs @lowderchris left in the code? We should make sure we have answers to those before merging.

Copy link
Member

@jmbhughes jmbhughes left a comment

Choose a reason for hiding this comment

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

I would like to see more tests. Is there a simple test case for IMAX matrix we can generate to actually check the values? We need to make sure the matrix is properly constructed. Or could we make sure its output is different when its applied than not applied. It would be particularly nice if the ratio of output with and without the IMAX effect has a nice property we can check for. I'm just brainstorming some tests here.

solpolpy/core.py Show resolved Hide resolved
solpolpy/core.py Show resolved Hide resolved
solpolpy/core.py Outdated Show resolved Hide resolved
solpolpy/core.py Outdated Show resolved Hide resolved
@jmbhughes jmbhughes self-requested a review March 5, 2024 16:39
Copy link
Member

@jmbhughes jmbhughes left a comment

Choose a reason for hiding this comment

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

Almost there. These should be easy things to fix I think.

solpolpy/core.py Outdated Show resolved Hide resolved
tests/test_core.py Outdated Show resolved Hide resolved
solpolpy/core.py Outdated Show resolved Hide resolved
@jmbhughes jmbhughes self-requested a review March 8, 2024 20:37
@jmbhughes jmbhughes merged commit 57c9362 into develop Mar 8, 2024
5 of 7 checks passed
@jmbhughes jmbhughes deleted the imax-effect-integration branch March 8, 2024 20:43
@jmbhughes jmbhughes removed their assignment Mar 14, 2024
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.

3 participants