-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add nuclear star cluster component #779
base: master
Are you sure you want to change the base?
Conversation
This is a Mac OS config file and should not be included in Galacticus.
Such files should never be added.
Adding comments from Matias here for reference:
and not related to my projects, I think there is a mistake in the |
Yes - definitely a mistake. I opened issue #781 for this and will push a fix. |
This seems reasonable. I'd suggest making it some fraction of the NSC stellar mass. |
I caught this problem in the PR refactoring black hole physics, and it is now implemented correctly there. We'll need to be sure to move the equivalent NSC behavior to this new class (after I merge in that PR). |
It would be better to move this to the |
* Renames files for consistency; * Conventionalizes citations; * Adds a glossary entry for nuclear star cluster (NSC); * Renames variables for clarity; * Reformats code for clarity.
* Variable renaming for clarity.
* Rename variables for clarity.
* Rename variables and files; * Fix formatting.
Mostly renaming variables and reformatting.
This maintains backward compatibility in default behavior
In accumulating stellar age data in the `NSC` component, we want to skip the calculation is no `NSC` exists. A type guard was missing which broke this logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Matias: I finished reviewing the PR. You've done an impressive amount of work on this, and the code looks good and clean overall. I also merged in the recent changes from the master
branch which move some of the black hole physics out of the nodeComponentBlackHole
classes into nodeOperators
and associated functions. This didn't require too much modification on your branch, so I suspect it will work successfully.
One important change. In the new NSC component, I changed:
<isDefault>true</isDefault>
to
<isDefault>false</isDefault>
This will cause Galacticus to default to a null
NSC component (i.e. no NSC), which means that the default behavior will be unchanged compared to earlier versions (without this, some models would break as they wouldn't have the correct parameters set for NSCs, and all models would have their results changed because they would now include NSC physics when previously the didn't).
I've left numerous review comments on the PR in GitHub. Some of these are quite minor (e.g. naming a constant instead of using a value directly, adding a citation, an unused property that could be removed, etc.). A few are pointing out possible bugs (e.g. wrong calculation of angular momentum, failure of mass conservation, etc.). Then a few are more structural - suggesting moving some behavior into separate classes, or making some property a meta-property. I'm happy to discuss these if anything isn't clear, but a couple of notes:
-
A "meta-property" is some property associated with a
nodeComponent
, but which isn't defined in thatnodeComponent
directly. Instead, it's attached to thenodeComponent
by some other class (often anodeOperator
) which needs to store some data in thenodeComponent
in order to do its calculation. Given the modular nature of Galacticus, this is useful as it means we don't have to carry around every property that could ever be needed by every module - we only create and use them if a module is actively used in a given model. This save memory and CPU time. -
When deciding what functionality to put into which classes, I think about it this way. Most behavior in Galacticus can be written as a differential equation. For example, star formation in the NSC could be written as:
where
There are three pieces to this:
- The properties involved,
$M_\star$ and$M_\mathrm{g}$ ; - The equations defining how this process (star formation) moves mass (or other quantities) between properties;
- The rate of the process,
$\phi(\cdots)$ .
In general:
- The properties themselves are defined in a
nodeComponent
; - The equations are defined in a
nodeOperator
; - The rate of the process is defined in its own class (e.g.
starFormationRateNuclearStarClusters
in this case).
Currently, for the nodeOperatorstellarCollisionsNSC
and nodeOperatorgasMassRateNSC
classes, the rates are also being implemented within the nodeOperator
. It would be better to move these into their own classes (so, more like how nodeOperatorStarFormationNuclearStarClusters
works by utilizing the starFormationRateNuclearStarClusters
class).
This will make it easier to use different physical models for the rates of these processes.
source/objects.nodes.components.NuclearStarCluster.standard.F90
Outdated
Show resolved
Hide resolved
source/objects.nodes.components.NuclearStarCluster.standard.F90
Outdated
Show resolved
Hide resolved
source/objects.nodes.components.nuclear_star_cluster.standard.bound_functions.Inc
Outdated
Show resolved
Hide resolved
source/objects.nodes.components.nuclear_star_cluster.standard.bound_functions.Inc
Show resolved
Hide resolved
<attributes isSettable="true" isGettable="true" isEvolvable="false" /> | ||
</property> | ||
<property> | ||
<name>age</name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only see this property set - it is never used. I assume the intent here is that it is output as a quantity of interest? If so, it might be better to make it a meta-properties (so it is only present in the output if requested). The same applies to the massCritical
property below.
else | ||
surfaceDensityGasNuclearStarCluster =+surfaceDensityGas(radiusNuclearStarCluster,massGasNuclearStarCluster) | ||
surfaceDensityGasNuclearStarClusterScaled=+surfaceDensityGasNuclearStarCluster & | ||
& /1.0d0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define 1.0d0
as a parameter here, e.g.:
double precision, parameter :: surfaceDensityGasNormalization=1.0d0 ! M☉/pc²
to make it more clear what we're doing - otherwise dividing by 1 seems pointless!
return | ||
end function surfaceDensityGas | ||
|
||
double precision function molecularFraction(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is essentially identical to the one in source/star_formation.rate_surface_density.disks.Krumholz2009.F90
. To avoid repeating code, we could move this function to a new module (in a new file), called Star_Formation_Rate_Krumholz2009_Utilities
or something similar - and then have both of these classes use the function from that new module.
! Get the metallicity in Solar units, and related quantities. | ||
metallicityRelativeToSolar=abundancesFuel%metallicity(metallicityTypeLinearByMassSolar) | ||
if (metallicityRelativeToSolar /= 0.0d0) then | ||
chi =+0.77d0*(1.0d0+3.1d0*metallicityRelativeToSolar**0.365d0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These calculations might also be good to put into a separate utility module (see below) since they are repeated in the disks Krumholz2009 star formation rate class.
abundancesFuel=nuclearStarCluster%abundancesGas() | ||
call abundancesFuel%massToMassFraction(massGasNuclearStarCluster) | ||
! Get the metallicity in Solar units, and related quantities. | ||
metallicityRelativeToSolar=abundancesFuel%metallicity(metallicityTypeLinearByMassSolar) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved metallicityRelativeToSolar
, chi
, and s
out of the class and made them local to this function, as they didn't need to be in the class directly. (In the disk Krumholz2009 class these values are memorized for re-use on subsequent calls, so it makes sense to define them in the class there).
Add nuclear star cluster model and BH seed formation scenario within NSCs due to stellar collisions.