Skip to content

Conversation

@ezapartas
Copy link
Contributor

…), merger center value calculation and allowing co_core_mass in detached

  • calculating center_gamma and avg_c_in_c_core for mergers whenever possible (as the core value if the cores do not merge and np.nap and mass-weighted average respectively, if the cores merge).
  • deepcopy of the merged_star object, not just a pointer to one of the two singlestar objects
  • more verbose in step_merged mainly, and step_detached
  • added co_core_mass as a potential option in the matching criteria in step_detached

@tassos25
Copy link
Contributor

tassos25 commented Jun 6, 2024

@dimsour94 and @kasdaglie should review this PR. @ezapartas has been using it and expects no issues.

@ezapartas ezapartas requested a review from dimsour94 June 6, 2024 14:43
@mkruckow mkruckow requested a review from kasdaglie July 25, 2024 14:59
@astroJeff
Copy link
Contributor

Update (July 25): Need to merge latest updates from development into this PR. Needs to be checked again by Manos.

Copy link
Contributor

@dimsour94 dimsour94 left a comment

Choose a reason for hiding this comment

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

Tested and operating well.

@astroJeff
Copy link
Contributor

Update (Aug 1): It looks like this code fixes the problem, but we need to be careful about the use of copy.deepcopy(star), as it may bloat the memory usage. @ezapartas and @dimsour94 to meet with @ka-rocha to check memory usage and see if either: it is fine with deep copy or if there is another way to perform the calculations.

@mkruckow mkruckow requested a review from ka-rocha August 8, 2024 09:01
@astroJeff
Copy link
Contributor

Update (Aug 29): Still holding off on this - we are opting for a different solution, where we don't rely on a copy command.

@astroJeff
Copy link
Contributor

Update (Sept 19): @dimsour94 to check with @ka-rocha and @ezapartas about the status of this code change. Will it cause any unintended consequences?

@ezapartas
Copy link
Contributor Author

ezapartas commented Sep 24, 2024

Update Sep 24: After discussing with @ka-rocha we decided to go with the solution of not creating or copying a new star object for the merged product. So firstly use the previous merged_star = star_base where we just reference the same Singlestar instance. BUT in order for this work, we need to calculate and change SIMULTANEOUSLY a few parameters of the merged star. Not one by one, as these are used in the further calculations. So maybe we would unpack the variables we will need for the calculation to copy them in new variables stored in the step instance, and then once we have calculated all the new parameters we update the old instance of S1 or S2.

@mkruckow mkruckow marked this pull request as draft March 20, 2025 15:19
@sgossage sgossage added the backlog This is sitting in our backlog of things to do. Action may be needed to pick back up. label Aug 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backlog This is sitting in our backlog of things to do. Action may be needed to pick back up.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants