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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring of Monte Carlo transport #2608

Merged
merged 12 commits into from
Jun 7, 2024

Conversation

andrewfullard
Copy link
Contributor

@andrewfullard andrewfullard commented May 6, 2024

馃摑 Description

Type: 馃 bugfix

Cleans up the Monte Carlo part of the code

  • Remove NumbaModel
  • Remove some deprecated options
  • Move spectrum to top level package

馃殾 Testing

How did you test these changes?

  • Testing pipeline
  • Other method (describe)
  • My changes can't be tested (explain why)

鈽戯笍 Checklist

  • I requested two reviewers for this pull request
  • I updated the documentation according to my changes
  • I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

Copy link

Check out this pull request on聽 ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@tardis-bot
Copy link
Contributor

tardis-bot commented May 8, 2024

*beep* *bop*

Hi, human.

The docs workflow has succeeded 鉁旓笍

Click here to see your results.

@andrewfullard andrewfullard marked this pull request as ready for review May 8, 2024 22:11
@andrewfullard
Copy link
Contributor Author

Best to get this reviewed and merged before it gets more complex.

@@ -37,9 +35,7 @@ def geometry(self):

@property
def model(self):
Copy link
Member

Choose a reason for hiding this comment

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

let's rename that property

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Asish can do this as part of his project

Copy link
Member

Choose a reason for hiding this comment

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

is this an issue?


Returns
-----------
v: float, current velocity
frac: float, dv/dr for current shell
"""
shell_id = r_packet.current_shell_id
v_inner = numba_model.v_inner[shell_id]
Copy link
Member

Choose a reason for hiding this comment

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

how did this work? Numba model only had time_explosion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't, this nonhomologous code is quite old now.

@tardis-bot
Copy link
Contributor

tardis-bot commented May 14, 2024

*beep* *bop*
Hi human,
I ran benchmarks as you asked comparing master (6fffa69) and the latest commit (7ba4317).
Here are the logs produced by ASV.
Results can also be downloaded as artifacts here.
Significantly changed benchmarks:

路 No results found

All benchmarks:

路 No results found

@wkerzendorf wkerzendorf merged commit 28a8eb6 into tardis-sn:master Jun 7, 2024
10 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.

None yet

4 participants