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.

Sign up for GitHub

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

Jump to bottom

Implementation of 1step and 2step analytical orbit spectra #66

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Implementation of 1step and 2step analytical orbit spectra #66

wants to merge 9 commits into from

Conversation

anvalen97
Copy link
Collaborator

@anvalen97 anvalen97 commented Jan 22, 2025 •
edited
Loading

Notes:

  • New packages were added, some of which turned out to be of no use. The essential ones for the new implementation are CSV, DataFrames, LoopVectorization, LegendrePolynomials and NaNMath. All the rest can be discarded actually.
  • weightsWebApp changes "had" to be committed, but can all be discarded (as you know).
  • One big change is how reactions are dealt with. I think identifying them by just the reactants is not enough, simply because you might want to look at one product or the other (the new cross sections account for this by flipping signs properly). I know you did not like the Reaction_name parameter, but that is what I could come up with for now (feel free to adjust this mechanic).
  • DT neutron cross section was upgraded to account for emission angle as well. Aka anisotropy.
  • Most comments I had for you regarding the new script, are situated in the first commented part of it. Proper documentation will be added as we go (including referencing to the papers etc).
  • Just a reminder that vcone.jl and spec.py were not actually touched by me :)

Lastly, I will work on including all remaining reactions, since for now it is just DT neutrons and alphaBe gammas. That should be fairly easy!

I will add other comments if needed.

@henrikjaerleblad henrikjaerleblad self-requested a review January 23, 2025 13:30
henrikjaerleblad
henrikjaerleblad reviewed Jan 23, 2025
View reviewed changes
henrikjaerleblad
henrikjaerleblad requested changes Jan 24, 2025
View reviewed changes
Copy link
Collaborator

@henrikjaerleblad henrikjaerleblad left a comment

Choose a reason for hiding this comment

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

Alright, so there are quite a lot of changes that need to be made before we can merge this branch with the main branch. However, let's team up to implement those changes together. I will start looking at the conflicts, to see what should be done. And maybe @anvalen97 can start going through all my comments?

@anvalen97
Copy link
Collaborator Author

anvalen97 commented Jan 24, 2025 via email

@henrikjaerleblad
Copy link
Collaborator

Ahaha hej there! I will start now with looking at your comments :) Slow and steady, I'm sure we'll make it! I appreciate your patience with going through all the code, line by line. I can do everything from github, right? Both replying to your comments and tweak the code where needed, no? Cheers, Andrea
________________________________ From: Henrik Järleblad @.> Sent: 24 January 2025 10:39 AM To: JuliaFusion/OWCF @.> Cc: Andrea Valentini @.>; Mention @.> Subject: Re: [JuliaFusion/OWCF] Implementation of 1step and 2step analytical orbit spectra (PR #66) @henrikjaerleblad commented on this pull request.
________________________________ In Project.toml<#66 (comment)>:
StaticArrays = "90137ffa-7385-5640-81b9-e52037218182"
+StructArrays = "09ab397b-f2b6-538f-b94a-2f83cf4a842a" One comment regarding how bad I have been myself in determining whether a package is necessary or not for the OWCF: Yesterday, I made a package clean-up where I removed OWCF packages that were no longer used/necessary. I am embarrassed to admit that I removed a total of more than 12 packages. 12! In short, knowing whether a package is necessary or not for the OWCF is not that easy. So no worries if you don't optimize it on your first try @anvalen97https://github.com/anvalen97 :) — Reply to this email directly, view it on GitHub<#66 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/BLBQZB3O7444NFTISTO5MHT2MIC6JAVCNFSM6AAAAABVVIYFROVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKNZSGE3DAMJUGY. You are receiving this because you were mentioned.Message ID: @.***>

No worries! I appreciate you taking the time to learn Julia and to develop the OWCF further :)

Yes, in theory you could edit the code directly on Github. You have to start a codespace however. I have not done so yet, because I want to have my local code to always reflect what is going on in the cloud. But if you can start a codespace, please go ahead. Otherwise, if I as an admin need to do it, please let me know !

@anvalen97
Copy link
Collaborator Author

anvalen97 commented Jan 24, 2025 via email

@anvalen97
Copy link
Collaborator Author

No worries! I appreciate you taking the time to learn Julia and to develop the OWCF further :)

Yes, in theory you could edit the code directly on Github. You have to start a codespace however. I have not done so yet, because I want to have my local code to always reflect what is going on in the cloud. But if you can start a codespace, please go ahead. Otherwise, if I as an admin need to do it, please let me know !

Hi again! I did not know what it was, but it looks interesting! I have booted up a codespace now, and it seems to be working, It's also quite handy because I can see code and comments altogether. I'll start now with replying (it'll take some time I think)

anvalen97
anvalen97 commented Jan 27, 2025
View reviewed changes
Copy link
Collaborator Author

@anvalen97 anvalen97 left a comment

Choose a reason for hiding this comment

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

I had everything ready in the codespace, and wasn't sure what to do.
Hence I just commit-pushed everything from there. I hope that's ok!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henrikjaerleblad henrikjaerleblad henrikjaerleblad requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anvalen97 @henrikjaerleblad