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

Smartstar #198

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

Smartstar #198

wants to merge 131 commits into from

Conversation

fearmayo
Copy link
Contributor

@fearmayo fearmayo commented Oct 14, 2022

Updates to the Active Particles code. Changes include porting across the supernova functionality that was part of the Star Particles machinery to the Smartstar flavour of Active Particles. I also pulled in Azton Wells changes that fixed the bug with supernovae but on accessing the bug fixes I don't think they are necessary as part of the Active Particles framework. In the AP framework the APs have their own grid and AP grids are always at the maximum refinement level. This should, unless I'm mistaken, negate the need for the bugfixes that are necessary in the Star Particle class as energy/metals/mass cannot be deposited into coarser grids as a result.

Unfortunately since I pulled in Azton's changes early on into my branch this PR has a mix of his changes and mine. I'm not sure there is an easy way now to back his changes out?

Actually looking through the file changes I think the easiest thing may be to just weed out the remaining changes that are specific to the Mechanical Stars (i.e. Azton's stuff) and then what is left will be specific to the Smartstars only. The merge history will show all of this stuff but maybe that's not such a bug deal?

@azton @jwise77 @bwoshea any thoughts?

azton and others added 30 commits August 9, 2019 15:06
…res and the deposition matches that of star_maker3mom.F now as well
…nicely with the weights, etc from Hopkins. Spherical clouds... do not.
…ese are fractions and dont need modification
…max(Nsn*1e51, eKinetic). made computational compromises for speed: winds are deposited NGP as opposed to CIC, SNe are still CIC. Star particles shut off after 500 Myr. Only one star particle forms per grid per timestep of that grid. No upper limit is placed on the amount of star formation added to new stars. Turned off critical_debug in the deposition, as it slows the code down too much with ~1k events.
@azton
Copy link

azton commented Oct 14, 2022

removing the Mechanical stars work sounds good to me. I can (at some point when time allows) put in a pull request for that specific functionality. The history will be more messy but the PR's will be more well defined and easy to follow.

@fearmayo
Copy link
Contributor Author

removing the Mechanical stars work sounds good to me. I can (at some point when time allows) put in a pull request for that specific functionality. The history will be more messy but the PR's will be more well defined and easy to follow.

It's more I'm wondering about whether your fix for the deposition bug in the Star Particle class is needed in the Active Particle implementation. I don't think it is but I could be wrong. We should probably try to get some of your work pulled in in the medium term though especially since your work does for sure fix the bug in the Star Particle class

@azton
Copy link

azton commented Oct 14, 2022

Ah. I am not familiar with how the active particles couple to the grid, so I'm not too useful in this context. From the StarParticle class side, I think the largest and most unpredictable error in the feedback coupling came from not normalizing the volume across grids before determining energy, etc, densities. The secondary (but potentially large) error came from estimating densities with a spherical volumes, then depositing onto non-spherical grid cell volumes. Even if the AP grid is forced to the maximum level, the second error could be significant (but might just fall into a "caveats" category). For the first error, so long as the deposition volume and density is consistent with all touched grids on the enzo side, it should be ok.

@bwoshea bwoshea self-requested a review May 11, 2023 21:21
@bwoshea
Copy link
Contributor

bwoshea commented May 12, 2023

@fearmayo (and @jwise77 , @azton) I've agreed to take the lead on getting this PR across the finish line. It seemed like there was some agreement in the discussion back in October about what to do with Azton's star particle deposition fixes - i.e., back those out and merge them in via a separate PR since they are not logically connected to this PR. Given that it's a useful bugfix and Azton has moved to a new position, I think that it's better to just keep it in this PR and rename the PR so it also makes it clear that there's a bugfix. What do people think?

@bwoshea
Copy link
Contributor

bwoshea commented May 12, 2023

@fearmayo you're going to need to update the PR so the tests stop failing. PR #210 and PR #211 fix a documentation issue with intersphinx and modify grackle's primary branch to be named main instead of master in the CI tests. If you merge in all changes up to the current tip of enzo's main things should start working again. Thank you!

@fearmayo
Copy link
Contributor Author

@fearmayo you're going to need to update the PR so the tests stop failing. PR #210 and PR #211 fix a documentation issue with intersphinx and modify grackle's primary branch to be named main instead of master in the CI tests. If you merge in all changes up to the current tip of enzo's main things should start working again. Thank you!

@bwoshea This PR has been merged with the main branch so it's passing the tests now. As for Azton's changes I backed them out sometime ago and they are no longer part of this PR. I did initially merge with Azton's branch but I later found that unnecessarily complicated and backed them out. This PR is more or less solely focused on the Smartstar setup.

@bwoshea
Copy link
Contributor

bwoshea commented Jun 30, 2023

Thanks, @fearmayo . I'll get to work on this soon. @jwise77 , are you also willing to review this PR in the next week or two?

@jwise77
Copy link
Contributor

jwise77 commented Jun 30, 2023

Can do!

@bwoshea bwoshea removed the request for review from azton February 21, 2024 17:39
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.

None yet

4 participants