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

Restructure/detailed balance foundation #2770

Conversation

wkerzendorf
Copy link
Member

No description provided.

…ace.py, transport_montecarlo_numba_interface.py, conftest.py, formal_integral.py, base.py, and macro_atom.py
@andrewfullard
Copy link
Contributor

Tests rely on tardis-sn/tardis-regression-data#24 merging first.

@andrewfullard andrewfullard marked this pull request as ready for review October 7, 2024 18:19
andrewfullard
andrewfullard previously approved these changes Oct 7, 2024
Copy link
Contributor

@andrewfullard andrewfullard left a comment

Choose a reason for hiding this comment

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

I'm happy with this, but I have also done a lot of work on it so it needs other reviews.

jvshields
jvshields previously approved these changes Oct 7, 2024
Copy link
Contributor

@jvshields jvshields left a comment

Choose a reason for hiding this comment

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

As far as I can tell it looks fine, but it's always hard to tell with the restructure PRs which parts are renames or functional changes worth paying attention to, or are just files and functions being moved around to places that make more sense.

@andrewfullard
Copy link
Contributor

As far as I can tell it looks fine, but it's always hard to tell with the restructure PRs which parts are renames or functional changes worth paying attention to, or are just files and functions being moved around to places that make more sense.

In this case @jvshields I think the tests are most illustrative because we compare this new structure with the previous way(s) of doing things.

@andrewfullard andrewfullard dismissed stale reviews from jvshields and themself via 571356d October 8, 2024 15:02
@andrewfullard
Copy link
Contributor

Tests are now failing for a different but related reason. electron_densities seems to stick around in memory for too long. Unclear why.

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 84.37500% with 40 lines in your changes missing coverage. Please review.

Project coverage is 70.70%. Comparing base (3a391ae) to head (71cc456).
Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
...asma/detailed_balance/rates/collision_strengths.py 76.36% 26 Missing ⚠️
...s/plasma/detailed_balance/rates/radiative_rates.py 31.57% 13 Missing ⚠️
...plasma/detailed_balance/rates/collisional_rates.py 97.22% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2770      +/-   ##
==========================================
- Coverage   70.87%   70.70%   -0.17%     
==========================================
  Files         209      213       +4     
  Lines       15589    15823     +234     
==========================================
+ Hits        11049    11188     +139     
- Misses       4540     4635      +95     

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

andrewfullard
andrewfullard previously approved these changes Oct 9, 2024
@andrewfullard andrewfullard merged commit 8106ede into tardis-sn:master Oct 14, 2024
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants