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

Revamping the hydrostatic mass profile product #1260

Closed
DavidT3 opened this issue Nov 19, 2024 · 17 comments
Closed

Revamping the hydrostatic mass profile product #1260

DavidT3 opened this issue Nov 19, 2024 · 17 comments
Assignees
Labels
enhancement New feature or request refinement If a feature has already been implemented, and works, but could do with another pass to improve it.

Comments

@DavidT3
Copy link
Owner

DavidT3 commented Nov 19, 2024

Work has already begun on this - it should be quite simple, I wish to revamp the HydrostaticMass profile class so that it reaches feature parity with the entropy profile class (which was reworked some months ago).

That means that you'll be able to create hydrostatic mass profiles from data points as well as fitted models, those data points can be interpolated if desired, and we'll also allow for projected temperature profiles to be passed as an approximation (we already allowed the projected temperature profiles to be fit with the temperature models if the user desires).

All these changes should make this class much more flexible and useful.

We'll also add some mass profile models to fit to the data driven hydrostatic mass profiles that will be possible (probably just simple NFW to begin with).

@DavidT3 DavidT3 added enhancement New feature or request refinement If a feature has already been implemented, and works, but could do with another pass to improve it. labels Nov 19, 2024
@DavidT3 DavidT3 self-assigned this Nov 19, 2024
DavidT3 added a commit that referenced this issue Nov 19, 2024
…y - the first part of the 'mass' method is essentially the same as the entropy method of SpecificEntropy, handling all the different ways we can get density and temperature information. The next part thing need to is implement derivative calculation, then add the actual hydrostatic mass calculation (making sure to support radius uncertainties like the old one did). For issue #1260
DavidT3 added a commit that referenced this issue Nov 19, 2024
…of the revamped hydrostatic mass profile. For issue #1260
DavidT3 added a commit that referenced this issue Nov 19, 2024
… setup) in the new hydro mass profile init. Also improved over the original, as apparently for the mass profile values the radius uncertainties were not being passed. For issue #1260
@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 19, 2024

NOTE that now the actual mass data points in the profile will have the radius uncertainty included in them - the trouble is that the radius 'uncertainty' for the profile points is set by the width of the bins of the input profiles, and whether that is really an 'uncertainty' in this respect I don't know.

May have to revisit

DavidT3 added a commit that referenced this issue Nov 19, 2024
…uding radii errors - have added it to the density model realisation approach (as it was in the original hydrostatic mass profile calculation). For issue #1260
@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 19, 2024

Figuring out the right way to include the radius errors in data is doing my head in:

  • For model based measurements it is fine, we'll do it the exact same way we already do in the existing HydrostaticMass class.
  • For data based profiles without interpolation turned on, I don't know that I can really do include the radius error in the density/temperature realisations - maybe I can in the gradient calculations though?
  • For data with interpolation turned on, I think it should be possible.
  • When it comes to the actual final mass calculation, there is a radius term in the actual equation, so will always be able to have the radius errors accounted for.

But yes it is a bit of a bloody mess

@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 19, 2024

Note to self - maybe force a refit of models to the temperature and density profiles (if the user goes that route) if the number of parameter samples and the number of samples specified on Hydrostatic mass profile declaration are not the same.

DavidT3 added a commit that referenced this issue Nov 19, 2024
… method should now use the radius distribution rather than a single value. For issue #1260
@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 19, 2024

Initial attempt to include the radii distribution in the density interpolation mode has ended in a HUGE failure, as I killed the kernel. Suspect the current setup is attempting to interpolate 'num_samples'$\times$'num_samples'**$\times$**number of radii. Which I think might cause some issues...

@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 19, 2024

I'm not actually sure I can include the radius distribution in the interpolation stage in the way I'm thinking - aaaargh

DavidT3 added a commit that referenced this issue Nov 19, 2024
…file calculations - only for the smooth model mode currently. For issue #1260
DavidT3 added a commit that referenced this issue Nov 20, 2024
…e the density gradient from non-interpolated data points in the new hydro mass setup. For issue #1260
DavidT3 added a commit that referenced this issue Nov 20, 2024
@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 20, 2024

Same deal with the derivatives, don't really think that I can include radius uncertainty, or that it would actually be valid to

@astrophysics-megan
Copy link

astrophysics-megan commented Nov 20, 2024 via email

DavidT3 added a commit that referenced this issue Nov 20, 2024
…lation but mismatched bins scenario) for the new hydrostatic mass calculation. For issue #1260
DavidT3 added a commit that referenced this issue Nov 20, 2024
… the opposite to the entropy calculation as we tend to use keV for that, so the conversion I copied over was going the wrong way around. For issue #1260
DavidT3 added a commit that referenced this issue Nov 20, 2024
…K/{insert distance unit}... think I might be tired. For issue #1260
DavidT3 added a commit that referenced this issue Nov 20, 2024
…ion array and the radii array (could well be a matrix transpose that I removed earlier - added it back in to test). For issue #1260
@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 20, 2024

For entropy and mass profiles, I will add a flag that allows them to contain unphysical values (i.e. negative masses) without erroring, though the default behaviour will still be to throw an error when it detects that.

@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 20, 2024

May alter the checking process in BaseAggregateProfile, where it looks at the type of the Python instances to check whether a set of profiles should be allowed to be plotted together - this is an issue currently because I want to compare the old hydrostatic mass profile to the new, but also would cause issues if you wanted to easily plot gas mass and hydro mass profile on the same axis for the same cluster (for instance). Should check that the units are compatible to plot on the same axis, rather than the absolute type of the class.

DavidT3 added a commit that referenced this issue Nov 20, 2024
…rofile class - means no error will be raised if negative mass is measured. Indirectly for issue #1260 (will now do the same for entropy whilst I think about it).
DavidT3 added a commit that referenced this issue Nov 20, 2024
…units are the same, rather than explicitly look at the Python instance type. For issue #1260
@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 20, 2024

Things are beginning to work in principle:

image

This is the comparison of the previous (orange) calculation and the new class calculation (blue) of hydrostatic mass from SMOOTH MODELS. So they should be identical, the only difference should be the radii at which they are sampled.

Maybe better illustrated by the 'joined points' plotting mode:

image

@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 20, 2024

Now for interpolating from data points, again comparing to the original measurement from the smooth models:

image

Same plot but 'joined points' plotting mode:

image

Broadly similar, which makes me more confident that the numerical differentiation is doing broadly what I thought it was (the documentation wasn't completely clear whether it wanted some coordinate analogue (i.e. radius - this is what I tried here) for the points, or if it wanted the separations between points (I think I'll try that as well to be safe).

Obviously the data-pointed derived profile is not as smooth, which was always going to happen - I do wonder however if there is a better way to handle interpolation, especially for the derivatives, I am currently interpolating the density/temperature AND THEN finding the gradient - I wonder if I should find the gradients of the original points and then interpolate that? That feels less valid somehow.

DavidT3 added a commit that referenced this issue Nov 21, 2024
… that were causing problems, to see if the fix works. For issue #1267 (and indirectly for issue #1260)
@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 21, 2024

Not a good fit, but the fits do actually work as intended now:

image

image

DavidT3 added a commit that referenced this issue Nov 21, 2024
…re and density. These are for the cases where there are more radii points for the mass profile than there are radii or density, and no interpolation. For issue #1260
DavidT3 added a commit that referenced this issue Nov 21, 2024
… mismatched radii and interpolation turned off (in the new hydrostatic mass profile implementation). For issue #1260
DavidT3 added a commit that referenced this issue Nov 21, 2024
…rivates work on the non-interpolation case. For issue #1260
DavidT3 added a commit that referenced this issue Nov 21, 2024
…f temp and dens non-interpolation mode in the new hydro mass implementation. For issue #1260
DavidT3 added a commit that referenced this issue Nov 21, 2024
DavidT3 added a commit that referenced this issue Nov 21, 2024
DavidT3 added a commit that referenced this issue Nov 21, 2024
… unit in a square bracket. For issue #1260 indirectly.
@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 21, 2024

New version of the mass calculation function cannot produce results for individual input radii when using anything but smooth model mode.

Actually in this vein check that the specific entropy class can produce individual values on demand.

DavidT3 added a commit that referenced this issue Nov 21, 2024
DavidT3 added a commit that referenced this issue Nov 21, 2024
…ly measure masses at custom radii properly. For issue #1260
DavidT3 added a commit that referenced this issue Nov 21, 2024
…ofile class can calculate distributions for a single radius input. For issue #1260
DavidT3 added a commit that referenced this issue Nov 21, 2024
…ofile class can calculate distributions for a single radius input. SECOND ATTEMPT. For issue #1260
DavidT3 added a commit that referenced this issue Nov 21, 2024
DavidT3 added a commit that referenced this issue Nov 21, 2024
DavidT3 added a commit that referenced this issue Nov 21, 2024
@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 21, 2024

Things are working now - I need to stop the user if they are trying to extrapolate with data points, that clearly won't work

DavidT3 added a commit that referenced this issue Nov 21, 2024
…a-point-driven mass profiles. Indirectly for issue #1260
DavidT3 added a commit that referenced this issue Nov 21, 2024
…tor to say that we can't yet calculate mass without a fitted model (though will add that). For issue #1271 (and indirectly #1260)
DavidT3 added a commit that referenced this issue Nov 22, 2024
…#1260, to let the new hydro mass profile refit a model if the number of samples doesn't match that specified in the init
DavidT3 added a commit that referenced this issue Nov 22, 2024
…s is found when declaring a mass profile now. For issue #1260
DavidT3 added a commit that referenced this issue Nov 22, 2024
… models can either be strings or model instances. Indirectly for issue #1260
DavidT3 added a commit that referenced this issue Nov 22, 2024
… models can either be strings or model instances. Indirectly for issue #1260
@DavidT3
Copy link
Owner Author

DavidT3 commented Nov 22, 2024

Issue #1271 is sort of related to this and left outstanding, but I am happy with the state of the new mass class, and am closing this issue.

@DavidT3 DavidT3 closed this as completed Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refinement If a feature has already been implemented, and works, but could do with another pass to improve it.
Projects
None yet
Development

No branches or pull requests

2 participants